mem: Reorganize cache tags and make them a SimObject
Review Request #1931 - Created June 20, 2013 and submitted
| Information | |
|---|---|
| Andreas Hansson | |
| gem5 | |
| default | |
| Reviewers | |
| Default | |
Changeset 9800:a66b30dc18fe --------------------------- mem: Reorganize cache tags and make them a SimObject This patch reorganizes the cache tags to allow more flexibility to implement new replacement policies. The base tags class is now a clocked object so that derived classes can use a clock if they need one. Also having deriving from SimObject allows specialized Tag classes to be swapped in/out in .py files. The cache set is now templatized to allow it to contain customized cache blocks with additional informaiton. This involved moving code to the .hh file and removing cacheset.cc. The statistics belonging to the cache tags are now including ".tags" in their name. Hence, the stats need an update to reflect the change in naming.
After some sedding of the stats (for tag stats include .tags in the name) all regressions pass
Posted (June 21, 2013, 11:33 a.m.)
In general this looks like a positive direction, but I'm concerned about keeping the python tag class and the cache template instance in sync. For example, it looks to me like if I specify a fully associative cache (via the cache's assoc parameter), but pass in an LRU tag SimObject, then the dynamic_cast in the tag initializer of the Cache constructor will fail, and I'll get a segfault in the constructor at 'tags->setCache(this);'. Not very user-friendly at best! One possibility would be to use dynamic_casts in BaseCacheParams::create() to figure out the type of the tags object and use the right Cache template instance accordingly. You lose the magic of letting the system pick the best tag instance for you (in the case where that makes sense) but at least you avoid the potential for a mismatch between the two types. Another possibility is to change the python perspective from there being two independent classes (Cache and LRU/FALRU) to an inheritance model, where you instantiate LRUCache or FALRUCache in python, and those map to Cache<LRU> or Cache<FALRU> in the C++. Then the python tag params become additional params on the derived Cache object.
-
src/mem/cache/tags/lru.hh (Diff revision 1) -
I'm guessing this violates Nate's include-sorting rules...
Review request changed
Updated (June 24, 2013, 8:51 a.m.)
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+291 -192) |
Posted (June 24, 2013, 10:52 a.m.)
-
src/mem/cache/tags/fa_lru.cc (Diff revision 2) -
Some body needs to explain to me what's going on in this part of the code. What' so sacrosanct about the number 17?
Posted (June 24, 2013, 11:14 a.m.)
-
src/mem/cache/tags/fa_lru.cc (Diff revision 2) -
I would argue this code is broken, but in any case it is unrelated to the current patch :-)
Posted (June 24, 2013, 11:29 a.m.)
-
src/mem/cache/base.cc (Diff revisions 1 - 2) -
I'd make this warning a little more explicit, e.g., "Consider using FALRU tags for a fully associative cache". Though if FALRU tags are still broken (as I think someone claimed), we may not want to do that yet.
Review request changed
Updated (June 25, 2013, 3:28 a.m.)
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+291 -192) |
Looks good! Thanks for the changes.