slicc: convert a variable to a pointer address

Information
Submitter:	Tony Gutierrez
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 10816:aebe70c731a7
---------------------------
slicc: convert a variable to a pointer address

Add the getPointer() SLICC function that simply adds an ampersand to the
variable so that it can be assigned to pointer.

Testing Done

I need an example why this is required.  What behavior is not achievable right now?

Brad Beckmann May 12, 2015, 4:49 p.m. (May 12, 2015, 4:49 p.m.)

Here is the initial patch description from a certain intern back in 2012:
"Adds a function to slicc to return the pointer of an object or reference.

This is needed in a few cases where you are not using a CacheMemory as the main store in

your controller. The other stores (PerfectCacheMemory and DirectoryMemory) return a reference

as their lookup functions which must be cast to a pointer to work with getCacheEntry"
Would it be sufficient if I just updated the desciption with this second paragraph?

LGTM. Sorry to harp on this, but once again, can you add info on this to the wiki?

Brad Beckmann May 12, 2015, 4:49 p.m. (May 12, 2015, 4:49 p.m.)

Jason you actually created this change during your internship (internal review posted on: 5/10/2012). You should add the info to the twiki. :)

I agree with Nilay on this one. If there is a reason to have a pointer, then the C++ code should export a pointer type. Brad, it seems you also agree. From your response on review 2790 ( http://reviews.gem5.org/r/2790/ ):

SLICC has been designed not to directly expose pointers to the programmer

So, this patch breaks a solid programming abstraction that already exists in SLICC. Further, if this patch were introduced and people use getPointer, then reverting this change later will be painful. It would be best to fix things the right way here.

Brad Beckmann May 18, 2015, 4:38 p.m. (May 18, 2015, 4:38 p.m.)

I don't think there is an easy way to avoid this unless one were to re-implement PerfectCacheMemor and DirectoryMemory.  That goes well beyond this patch and has many implications.  This patch has existed for 3+ years because there is no easy fix here.  Woud you be ok if we changed the name of the function from getPointer() to convertDirEntryToCacheEntry()?

Brad Beckmann May 28, 2015, 5:02 p.m. (May 28, 2015, 5:02 p.m.)

On 5/19 Nilay said "No.
Can you explain how DirectoryMemory and PerfectCacheMemory are different from CacheMemory?  To me the fact that the patch has existed for 3+ years means that nobody tried to do things the way I feel they should be done. 

Please maintain regularity across different structures.  I do not want to treat DirectoryMemory any different from CacheMemory."
The difference is all three structures have lookup functions that return different data types.  The function getCacheEntry assumes lookup returns a CacheEntry pointer.
This is a simple change that helps fix a problem we are having and doesn't impact other protocols.  Please do not protest just because it wasn't done the way you would have done it.  There is only one Nilay and there are many issues encountered when developing protocols that cannot be solved by a single person.

Joel Hestness May 29, 2015, 7:48 a.m. (May 29, 2015, 7:48 a.m.)

Brad, you seem pretty overzealous here: Nilay's not the only one that opposes this change: I do too, and as I pointed out before, so do you. By our powers combined, surely we can solve 3 times as many issues and actually robustify the codebase rather than just tacking on special features.

PerfectCacheMemory is the only odd ball. The right fix was trivial and actually cleans up a very old SLICC bug. Please consider this patch instead: http://reviews.gem5.org/r/2862/

Description:

~		Changeset 10843:632faa18dad5
	~	Changeset 10816:aebe70c731a7

		slicc: convert a variable to a pointer address

		Add the getPointer() SLICC function that simply adds an ampersand to the
		variable so that it can be assigned to pointer.

Diff:

Revision 2 (+51)

Show changes

	src/mem/slicc/parser.py
	src/mem/slicc/ast/GetPointerAST.py
	src/mem/slicc/ast/__init__.py

You have a pending review.

Review Board 2.0.15

This change has been discarded.

Screenshots

Files

Description:

Changeset 10843:632faa18dad5

Changeset 10816:aebe70c731a7

Diff:

Status: Discarded