----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/358/#review596 -----------------------------------------------------------
Hi Nilay, First, I must say this is an impressive amount of work. You definitely got a lot done over holiday break. :) Overall, this set of patches is definitely close, but I want to see if we can take them a step forward. Also I have a few suggestions that may make things easier. Finally, I have a bunch of minor questions/suggestions on individual lines, but I’ll hold off on those until you can respond to my higher-level questions. The main thing I would like to see improved is not having to differentiate between “entry” and “entry_ptr” in the .sm files. Am I correct that the only functions in the .sm files that are passed an “entry_ptr” are “is_valid_ptr”, “getCacheEntry”, and “set_cache_entry”? If so, it seems that all three functions are generated with unique python code, either in an AST file or StateMachine.py. Therefore, could we just pass these functions “entry” and rely on the underneath python code to generate the correct references? This would make things more readable, “is_valid_ptr()” becomes “is_valid”, and it doesn’t require the slicc programmer to understand which functions take an entry pointer versus the entry itself. If we can’t make such a change, I worry about how much extra complexity this change pushes on the slicc programmer. Also another suggestion to make things more readable, please replace the name L1IcacheMemory_entry with L1I_entry. Do the same for L1D_entry and L2_entry. That will shorten many of your lines. So am I correct that hammer’s simultaneous usage of valid L1 and L2 cache entries in certain transitions is the only reason that within all actions, the getCacheEntry calls take multiple cache entries? If so, I think it would be fairly trivial to use a tbe entry as an intermediary between the L1 and L2 for those particular hammer transitions. That way only one cache entry is valid at any particular time, and we can simply use the variable cache_entry in the actions. That should clean things up a lot. By the way, once you check in these patches, the MESI_CMP_directory protocol will be deprecated, correct? If so, make sure you include a patch that removes it from the regression tester. Brad - Brad On 2011-01-03 14:18:13, Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/358/ > ----------------------------------------------------------- > > (Updated 2011-01-03 14:18:13) > > > Review request for Default. > > > Summary > ------- > > The purpose of this patch is to change the way CacheMemory interfaces with > coherence protocols. Currently, whenever a cache controller (defined in the > protocol under consideration) needs to carry out any operation on a cache > block, it looks up the tag hash map and figures out whether or not the block > exists in the cache. In case it does exist, the operation is carried out > (which requires another lookup). Over a single clock cycle, multiple such > lookups take place as observed through profiling of different protocols. It > was seen that the tag lookup takes anything from 10% to 20% of the simulation > time. In order to reduce this time, this patch is being posted. The > CacheMemory class now will have a function that will return pointer to a > cache block entry, instead of a reference (though the function that returns > the reference has been retained currently). Functions have been introduced > for setting/unsetting a pointer and check its pointer. Similar changes have > been carried out for Transaction Buffer entries as well. > > Note that changes are required to both SLICC and the protocol files. This > patch carries out changes to SLICC and committing this patch alone, I > believe, will ___break___ all the protocols. I am working on patching the > protocols as well. This patch is being put to get feed back from other > developers. > > > Diffs > ----- > > src/mem/protocol/RubySlicc_Types.sm UNKNOWN > src/mem/ruby/slicc_interface/AbstractCacheEntry.hh UNKNOWN > src/mem/ruby/slicc_interface/AbstractCacheEntry.cc UNKNOWN > src/mem/ruby/system/CacheMemory.hh UNKNOWN > src/mem/ruby/system/CacheMemory.cc UNKNOWN > src/mem/ruby/system/TBETable.hh UNKNOWN > src/mem/slicc/ast/ActionDeclAST.py UNKNOWN > src/mem/slicc/ast/FormalParamAST.py UNKNOWN > src/mem/slicc/ast/FuncCallExprAST.py UNKNOWN > src/mem/slicc/ast/InPortDeclAST.py UNKNOWN > src/mem/slicc/ast/MethodCallExprAST.py UNKNOWN > src/mem/slicc/ast/TypeDeclAST.py UNKNOWN > src/mem/slicc/ast/__init__.py UNKNOWN > src/mem/slicc/parser.py UNKNOWN > src/mem/slicc/symbols/StateMachine.py UNKNOWN > > Diff: http://reviews.m5sim.org/r/358/diff > > > Testing > ------- > > I have tested these changes using the MOESI_CMP_directory and MI protocols. > > > Thanks, > > Nilay > >
_______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
