Hi Nilay, Lisa Hsu (another member of the lab here at AMD) and I were discussing these changes a bit more and there was one particular idea that came out of our conversation that I wanted to relay to you. Basically, we were thinking about how these changes will impact the flexibility of SLICC and we concluded that it is important to allow one to craft custom getCacheEntry functions for each protocol. I know initially I was hoping to generate these functions, but I now don’t think that is possible without restricting what protocols can be support by SLICC. Instead we can use these customized getCacheEntry functions to pass the cache entry to the actions via the trigger function. For those controllers that manage multiple cache memories, it is up to the programmer to understand what the cache entry pointer points to. That should eliminate the need to have multiple *cacheMemory_entry variables in the .sm files. Instead there is just the cache_entry variable that is set either by the trigger function call or set_cache_entry.
Does that make sense to you? Brad From: Nilay Vaish [mailto:ni...@cs.wisc.edu] Sent: Tuesday, January 04, 2011 9:43 AM To: Nilay Vaish; Default; Beckmann, Brad Subject: Re: Review Request: Changing how CacheMemory interfaces with SLICC This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/358/ On January 3rd, 2011, 3:31 p.m., Brad Beckmann wrote: 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 > 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. There are functions that are passed cache entry and transaction buffer entry as arguments. Currently, I assume that these arguments are passed using pointers. > > 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. The names of the cache entry variables are currently tied with the names of the cache memory variables belonging to the machine. If the name of the cache memory variable is A, then the corresponding cache entry variable is named A_entry. > 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. Oops! Should have thought of that before doing all those changes. But can we assume that we would always have only one valid cache entry pointer at any given time? If that's true, I would probably revert to previous version of the patch. This should also resolve the naming issue. > 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. I have a patch for the protocol, but I need to discuss it. Do you think it is possible that a protocol is not in a dead lock but random tester outputs so because the underlying memory system is taking too much time? The patch works for 1, 2, and 4 processors for 10,000,000 loads. I have tested these processor configurations with 40 different seed values. But for 8 processors, random tester outputs some thing like this -- panic: Possible Deadlock detected. Aborting! version: 6 request.paddr: 12779 m_writeRequestTable: 15 current time: 369500011 issue_time: 368993771 difference: 506240 @ cycle 369500011 [wakeup:build/ALPHA_SE_MESI_CMP_directory/mem/ruby/system/Sequencer.cc, line 123] - Nilay On January 3rd, 2011, 2:18 p.m., Nilay Vaish wrote: Review request for Default. By Nilay Vaish. Updated 2011-01-03 14:18:13 Description 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. Testing I have tested these changes using the MOESI_CMP_directory and MI protocols. 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) View Diff<http://reviews.m5sim.org/r/358/diff/>
_______________________________________________ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
