Change interface between coherence protocols and CacheMemory
Review Request #358 - Created Dec. 21, 2010 and submitted
| Information | |
|---|---|
| Nilay Vaish | |
| gem5 | |
| default | |
| Reviewers | |
| Default | |
Change interface between coherence protocols and CacheMemory 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). As observed through profiling of different protocols, multiple such lookups take place for a given cache block. It was noted 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. Changes to CacheMemory, TBETable and AbstractCacheEntry classes: 1. The lookup function belonging to CacheMemory class now returns a pointer to a cache block entry, instead of a reference. The pointer is NULL in case the block being looked up is not present in the cache. Similar change has been carried out in the lookup function of the TBETable class. 2. Function for setting and getting access permission of a cache block have been moved from CacheMemory class to AbstractCacheEntry class. 3. The allocate function in CacheMemory class now returns pointer to the allocated cache entry. Changes to SLICC: 1. Each action now has implicit variables - cache_entry and tbe. cache_entry, if != NULL, must point to the cache entry for the address on which the action is being carried out. Similarly, tbe should also point to the transaction buffer entry of the address on which the action is being carried out. 2. If a cache entry or a transaction buffer entry is passed on as an argument to a function, it is presumed that a pointer is being passed on. 3. The cache entry and the tbe pointers received __implicitly__ by the actions, are passed __explicitly__ to the trigger function. 4. While performing an action, set/unset_cache_entry, set/unset_tbe are to be used for setting / unsetting cache entry and tbe pointers respectively. 5. is_valid() and is_invalid() has been made available for testing whether a given pointer 'is not NULL' and 'is NULL' respectively. 6. Local variables are now available, but they are assumed to be pointers always. 7. It is now possible for an object of the derieved class to make calls to a function defined in the interface. 8. An OOD token has been introduced in SLICC. It is same as the NULL token used in C/C++. If you are wondering, OOD stands for Out Of Domain. 9. static_cast can now taken an optional parameter that asks for casting the given variable to a pointer of the given type. 10. Functions can be annotated with 'return_by_pointer=yes' to return a pointer. 11. StateMachine has two new variables, EntryType and TBEType. EntryType is set to the type which inherits from 'AbstractCacheEntry'. There can only be one such type in the machine. TBEType is set to the type for which 'TBE' is used as the name.
These changes have been tested with five different protocols, namely, MI, MOESI CMP directory, token, MOESI hammer and MESI CMP directory. The testing was carried out using ruby_random_tester.py. Multiple random seeds were used, number of loads was varied from 4,000,000 to 10,000,000, number of processors was varied from 1 to 4.
Hi Nilay, Could you please include the changes to the MOESI_CMP_directory protocol in this patch? It is difficult to understand the ramifications of these changes without seeing their impact to the .sm files. Also this is a very tricky patch and the holiday break is approaching, so please be patient. I will try to get you my feedback as soon as I can. Brad
Change Summary:
The functions is_valid_cache_entry() and is_valid_tbe() have been removed from the Controller class. Instead, these have been replaced with a unified call to is_valid_ptr(<ptr variable>). This new call is handled in the same fashion as check_allocate(). Functions changePermission() and getPermission() have been removed from the CacheMemory class.
Testing Done: |
|
||||||
|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+526 -112) |
Change Summary:
1. lookup_ptr() function has been removed from CacheMemory class. Instead, lookup() function has been changed so that it returns a pointer to an AbstractCacheEntry, as opposed to a reference. 2. The manner in which the declaration of a CacheEntry structure in the protocol file is detected has been changed. Earlier detection was carried out using the name of the structure i.e. the structure should be named 'Entry' for the compiler to assume that it is the CacheEntry structure for that particular machine under consideration. Now, the check is made on the interface supplied with the structure declaration. It should be 'AbstractCacheEntry'. I think an assumption has been made that this would happen only once in the entire file. An error is raised if it happens more than once, though the error condition has not yet been tested. 3. While making changes to the MOESI Hammer protocol, I realized that the previously made changes to SLICC would not suffice. Earlier, the changes had been made under the assumption that at any given point of time, only one cache entry would be updated. But in the Hammer protocol, two cache entries can be processed in a single step. One of these would belong to L1 and the other to L2. I think the Hammer protocol could not have been supported with just one cache entry variable being passed to the trigger function. So, now the number of cache entries passed to the trigger function and subsequently to actions is same as the number of cache memories in the machine under consideration. This is two for MOESI_CMP_* and three for MOESI_hammer. Most of the changes that have been made since the previous version of the diff, are there to support because of the reason outlined above.
-
src/mem/ruby/slicc_interface/AbstractCacheEntry.hh (Diff revision 4) -
It looks like you're also changing the lock flag from being a separate table to a cache entry field... this seems reasonable, but also seems independent of the other changes. I don't know that it's worth separating into separate changeset, but there should be some mention of this in the commit message I think.
-
src/mem/slicc/ast/ActionDeclAST.py (Diff revision 4) -
The expression '"%s" % foo' is a pretty unconventional way to force foo to be a string :-). If foo is already a string (which might be the case here) you don't need it at all. If foo is not a string, the conventional way to force a conversion is 'str(foo)'. I see there are a number of places where you do this.
Change Summary:
I have made the changes that Steve suggested for converting an expression in to a string.
Diff: |
Revision 5 (+490 -130)
|
|---|
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
Change Summary:
This revision has following changes from the previous version - 1. The most important change is that we now formally assume that if multiple cache memories under the controller of a single controller, then for any given address, at most one of them can hold a cache entry for it. This simplifies a lot many things. We are now required to work with only one cache entry pointer. This pointer is set to the cache entry for the address being passed to the trigger function. The checks that have to carried out for code generation also get simplified. 2. is_invalid() has been introduced that can be used for testing whether a given entry (cache or transaction buffer) == NULL. 3. Earlier, *_ptr variables where being added on declaration of methods (which make use of TBE and / or Cache Entry), actions and in-ports. This is not being done any longer. 4. In FuncCallExprAST.py, when function parameters are passed to a function, a check has been added for expressions that are being passed as parameters. If the expression has a type TBE or AbstractCacheEntry, then all the occurrences of '*' are removed from the expression. This is under the assumption that the code from which the function call is being made has pointers to the variables being passed. Since SLICC will replace any occurrence of these variables with dereferenced version, therefore it is required that '*' be removed before the variable is passed to the function.
Diff: |
Revision 6 (+476 -144)
|
|---|
Diff: |
Revision 9 (+525 -142)
|
|---|
I can't fully appreciate these latest changes without also seeing the updated .sm files, but overall this looks very inline with what we have been discussing over the past few days. I just have one basic question on why we need to overload doTransition, getState, and setState. Why can't we just have one version? Also a possibly related idea is can you insert asserts (!= NULL) for the tbe and cache_entry pointers before they are dereferenced in the generated code?
-
src/mem/slicc/ast/FuncCallExprAST.py (Diff revision 9) -
Why do we need four versions of doTransition? Can't we just pass in NULL/OOD for the tbe and cache_entry?
-
src/mem/slicc/symbols/StateMachine.py (Diff revision 9) -
After you make your change, why would getState ever be called without the address, tbe, and cache_entry? Wouldn't you just pass in NULL/OOD if the tbe or cache_entry didn't exist?
-
src/mem/slicc/symbols/StateMachine.py (Diff revision 9) -
After you make this change, why would setState ever be called without passing the address, tbe, and cache_entry?
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Branch: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 10 (+532 -143)
|
Testing Done: |
|
|---|
Hi Nilay, This patch looks great! Once you take care of the missing headers, consider it done from my perspective. I believe this was mentioned before, but it would be best if you merged all the protocol patches with this patch and checked them in as one patch. Brad
-
src/mem/slicc/ast/IsValidPtrExprAST.py (Diff revision 10) -
Please add the license header and Wisconsin copyright to this file.
-
src/mem/slicc/ast/LocalVariableAST.py (Diff revision 10) -
Please add the license header and Wisconsin copyright to this file.
-
src/mem/slicc/ast/OodAST.py (Diff revision 10) -
Please add the license header and Wisconsin copyright to this file.
