On 2011-01-03 15:31:20, 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
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/358/#review596
-----------------------------------------------------------
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
m5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/m5-dev
