ruby: handle llsc accesses through CacheEntry, not CacheMemory

Information
Submitter:	Nilay Vaish
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 10995:3e7942da8ea3
---------------------------
ruby: handle llsc accesses through CacheEntry, not CacheMemory

The sequencer takes care of llsc accesses by calling upon functions
from the CacheMemory.  This is unnecessary once the required CacheEntry object
is available.  Thus some of the calls to findTagInSet() are avoided.

Testing Done

Issue Summary

Description	From	Last Updated	Status
Should this be "if (e && ..."?	Joel Hestness	July 12, 2015, 8:37 a.m.	Resolved
To keep things clean, could we also remove the clearLocked, isLocked, and setLocked functions from the CacheMemory, since they are ...	Joel Hestness	July 16, 2015, 10:29 a.m.	Resolved
One issue with moving the implementation to the AbstractEntry is we lose the DPRINTF which was in the CacheMemory implementation. ...	Jason Lowe-Power	Aug. 3, 2015, 8:22 a.m.	Resolved

This looks alright to me. Just a couple small things below.

src/mem/ruby/system/Sequencer.cc (Diff revision 1)

Should this be "if (e && ..."?

Show all issues

Nilay Vaish July 12, 2015, 8:42 a.m. (July 12, 2015, 8:42 a.m.)
```
Oops!
```

src/mem/ruby/system/Sequencer.cc (Diff revision 1)

To keep things clean, could we also remove the clearLocked, isLocked, and setLocked functions from the CacheMemory, since they are only used where you've changed code?

Show all issues

Nilay Vaish July 12, 2015, 8:42 a.m. (July 12, 2015, 8:42 a.m.)

Somebody might be using them. It does not hurt us at all to leave them in place.

Joel Hestness July 12, 2015, 9:02 a.m. (July 12, 2015, 9:02 a.m.)

Sure. In that case, could you add a comment near their declarations in CacheMemory that the suggested functions for handling LL/SC are now in CacheEntry?

Jason Lowe-Power Aug. 3, 2015, 8:03 a.m. (Aug. 3, 2015, 8:03 a.m.)

To me, it seems dangerous to leave the functions in CacheMemory. Correct me if I'm wrong, but if you call m_dataCache_ptr->setLocked() and then e->clearLocked() you'll have a major problem. Said another way, if you combine using the cache entry and the cache memory implementations it won't work. 

My suggestion: Implement CacheMemory::*Locked by calling the AbstractEntry functions.

Jason Lowe-Power Aug. 3, 2015, 8:12 a.m. (Aug. 3, 2015, 8:12 a.m.)

> Nilay said:
> As of now, I think things would work fine since both the functions work on
> the same variable.

> > My suggestion: Implement CacheMemory::*Locked by calling the AbstractEntry functions.

> I'll make this change.

You're right. Thanks for making the change. It'll prevent people from making the same mistake I did! :)

One more small thing, but other than that it looks good to me.

Jason Lowe-Power Aug. 3, 2015, 8:17 a.m. (Aug. 3, 2015, 8:17 a.m.)

Are there any regressions that cover this code? Do we have an alpha(?) test that uses LLSC? It would be nice to see this code tested, just in case we have all missed something.

src/mem/ruby/slicc_interface/AbstractCacheEntry.hh (Diff revision 1)

One issue with moving the implementation to the AbstractEntry is we lose the DPRINTF which was in the CacheMemory implementation. Could you move that DPRINTF to here?

Show all issues

Description:

~		Changeset 10928:2ff7814c0484
	~	Changeset 10995:3e7942da8ea3

		ruby: handle llsc accesses through CacheEntry, not CacheMemory

		The sequencer takes care of llsc accesses by calling upon functions
		from the CacheMemory. This is unnecessary once the required CacheEntry object
		is available. Thus some of the calls to findTagInSet() are avoided.

Diff:

Revision 2 (+52 -19)

Show changes

	src/mem/ruby/slicc_interface/AbstractCacheEntry.hh
	src/mem/ruby/slicc_interface/AbstractCacheEntry.cc
	src/mem/ruby/structures/CacheMemory.hh
	src/mem/ruby/structures/CacheMemory.cc
	src/mem/ruby/system/Sequencer.cc

Thanks for the updates!

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 10928:2ff7814c0484

Changeset 10995:3e7942da8ea3

Diff:

Status: Closed (submitted)