mem: Remove redundant is_top_level cache parameter

Information
Submitter:	Andreas Hansson
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 10887:9be908d7c501
---------------------------
mem: Remove redundant is_top_level cache parameter

This patch takes the final step in removing the is_top_level parameter
from the cache. With the recent changes to read requests and write
invalidations, the parameter is no longer needed, and consequently
removed.

This also means that asymmetric cache hierarchies are now fully
supported (and we are actually using them already with L1 caches, but
no table-walker caches, connected to a shared L2).

Testing Done

Issue Summary

Description	From	Last Updated	Status
What's going on over here that's related to the aim of the patch?	Nilay Vaish	June 29, 2015, 2:39 a.m.	Dropped
Same question as previous one?	Nilay Vaish	June 29, 2015, 2:41 a.m.	Resolved

Overall I am fine. I would probably read the patch again after your response
to questions below and mark it to be shipped.

src/mem/cache/cache_impl.hh (Diff revision 1)

What's going on over here that's related to the aim of the patch?

Show all issues

Andreas Hansson June 29, 2015, 2:43 a.m. (June 29, 2015, 2:43 a.m.)

I have expanded the comment. We need to determine if the read came from a cache or not, and we do so by looking at the type of read command. The ones in the expression are from a cache.

src/mem/cache/cache_impl.hh (Diff revision 1)

Same question as previous one?

Show all issues

Andreas Hansson June 29, 2015, 2:43 a.m. (June 29, 2015, 2:43 a.m.)

Arguably, the new check is actually more sensible that what we had previously, since in cases where it was top level, but forward snoops was true it would have been more kosher to actually forward the snoop.

Nilay Vaish July 1, 2015, 9:58 p.m. (July 1, 2015, 9:58 p.m.)

So now you will forward the snoop to the cpu? What does that mean?
Instead, should you not mark forward_snoops as False for L1 caches?

Andreas Hansson July 2, 2015, 6:51 a.m. (July 2, 2015, 6:51 a.m.)

You must indeed forward snoops to the core. This is needed for the synchronisation operations stored in the LSQ.

This has always been the case.

Description:

~		Changeset 10877:5715b110f655
	~	Changeset 10887:9be908d7c501

		mem: Remove redundant is_top_level cache parameter

		This patch takes the final step in removing the is_top_level parameter
		from the cache. With the recent changes to read requests and write
		invalidations, the parameter is no longer needed, and consequently
		removed.

		This also means that asymmetric cache hierarchies are now fully
		supported (and we are actually using them already with L1 caches, but
		no table-walker caches, connected to a shared L2).

Diff:

Revision 2 (+15 -29)

Show changes

	configs/common/Caches.py
	configs/common/O3_ARM_v7a.py
	configs/example/memcheck.py
	configs/example/memtest.py
	src/mem/cache/BaseCache.py
	src/mem/cache/base.hh
	src/mem/cache/base.cc
	src/mem/cache/cache_impl.hh

Ship It!

I like the idea of adding a FromCache attribute in packet.cc; my concern with the current setup is that, if someone added a new cache command, there wouldn't be anything to indicate to them that they should be adding another condition to this compound if.

Other than that, looks good!

Andreas Hansson June 30, 2015, 9:21 p.m. (June 30, 2015, 9:21 p.m.)

Sure. Is it ok if we add this as a second patch? I feel like that is a big enough change that it deserves a separate changeset.

Steve Reinhardt June 30, 2015, 10:35 p.m. (June 30, 2015, 10:35 p.m.)
```
Yea, that's fine... consider this a 'ship it' then.
```

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 10877:5715b110f655

Changeset 10887:9be908d7c501

Diff:

Status: Closed (submitted)