----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/611/#review1111 -----------------------------------------------------------
Hi Nilay, This patch is getting close. Comments below. There is one item in this patch that I would request other developers to comment on. Please look at Nilay's changes to packet.hh and let us know if they seem reasonable to you. The main point here is that functional requests are not guaranteed to succeed in Ruby and thus there must be some mechanism to indicate when they failed. Does passing that information back in the packet make sense? Should it be in the request instead? Other possible issues? configs/example/ruby_mem_test.py <http://reviews.m5sim.org/r/611/#comment1495> It seems that the following three parameters should not be hardcoded, but instead should be set using command line options. Hardcoding the atomic parameter to false still makes sense, since Ruby can handle atomic requests. Also we should update the comment above. src/cpu/testers/memtest/memtest.cc <http://reviews.m5sim.org/r/611/#comment1496> Remove commented out line src/mem/packet.hh <http://reviews.m5sim.org/r/611/#comment1497> This is more of a question to the other developers: Is this a reasonable way to return functional request failure? It works for me, but I want to get others opinion. src/mem/protocol/MESI_CMP_directory-L1cache.sm <http://reviews.m5sim.org/r/611/#comment1498> Why are you adding this function? SLICC already generates a similar function: getPermission(). Overall, I hope/think we can add functional access support without requiring any more changes to the protocol specific .sm files beyond the changeset: 8086:bf0335d98250 that I checked in a couple months ago. src/mem/ruby/system/CacheMemory.cc <http://reviews.m5sim.org/r/611/#comment1499> Why are you commenting this line out? If you think it is no longer needed, just remove it. If we remove it, can we guarentee that the permission is initialized to some value? For instance, do we know whether the "entry" constructor will allows initialize the value? src/mem/ruby/system/DirectoryMemory.cc <http://reviews.m5sim.org/r/611/#comment1500> Please align this statement. src/mem/ruby/system/RubyPort.cc <http://reviews.m5sim.org/r/611/#comment1501> Overall, I really like the comments you added in this file. They really help clarify what is going on here. Just one minor correction: change "Any copy" to "Any valid copy" src/mem/ruby/system/System.hh <http://reviews.m5sim.org/r/611/#comment1502> Why are these functions static? I'm concerned that declaring these static will unecessarily restrict using Ruby in multiple system simulation. Also from my understanding of the code, there is no need to make them static. Perhaps I'm missing something. - Brad On 2011-04-12 16:35:34, Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/611/ > ----------------------------------------------------------- > > (Updated 2011-04-12 16:35:34) > > > Review request for Default. > > > Summary > ------- > > Ruby: Add support for functional accesses > This patch is meant for aiding discussions on implementation of functional > access support in Ruby. > > > Diffs > ----- > > configs/example/ruby_mem_test.py cb1e137ac35e > configs/ruby/MESI_CMP_directory.py cb1e137ac35e > configs/ruby/Ruby.py cb1e137ac35e > src/cpu/testers/memtest/memtest.cc cb1e137ac35e > src/mem/packet.hh cb1e137ac35e > src/mem/packet.cc cb1e137ac35e > src/mem/protocol/MESI_CMP_directory-L1cache.sm cb1e137ac35e > src/mem/protocol/MESI_CMP_directory-L2cache.sm cb1e137ac35e > src/mem/protocol/MESI_CMP_directory-dir.sm cb1e137ac35e > src/mem/protocol/RubySlicc_Types.sm cb1e137ac35e > src/mem/ruby/network/Network.cc cb1e137ac35e > src/mem/ruby/network/Network.py cb1e137ac35e > src/mem/ruby/profiler/Profiler.cc cb1e137ac35e > src/mem/ruby/profiler/Profiler.py cb1e137ac35e > src/mem/ruby/recorder/Tracer.cc cb1e137ac35e > src/mem/ruby/recorder/Tracer.py cb1e137ac35e > src/mem/ruby/system/AbstractMemory.hh PRE-CREATION > src/mem/ruby/system/AbstractMemory.cc PRE-CREATION > src/mem/ruby/system/AbstractMemory.py PRE-CREATION > src/mem/ruby/system/Cache.py cb1e137ac35e > src/mem/ruby/system/CacheMemory.hh cb1e137ac35e > src/mem/ruby/system/CacheMemory.cc cb1e137ac35e > src/mem/ruby/system/DirectoryMemory.hh cb1e137ac35e > src/mem/ruby/system/DirectoryMemory.cc cb1e137ac35e > src/mem/ruby/system/DirectoryMemory.py cb1e137ac35e > src/mem/ruby/system/RubyPort.hh cb1e137ac35e > src/mem/ruby/system/RubyPort.cc cb1e137ac35e > src/mem/ruby/system/RubySystem.py cb1e137ac35e > src/mem/ruby/system/SConscript cb1e137ac35e > src/mem/ruby/system/Sequencer.cc cb1e137ac35e > src/mem/ruby/system/Sequencer.py cb1e137ac35e > src/mem/ruby/system/System.hh cb1e137ac35e > src/mem/ruby/system/System.cc cb1e137ac35e > src/mem/slicc/ast/MemberExprAST.py cb1e137ac35e > > Diff: http://reviews.m5sim.org/r/611/diff > > > Testing > ------- > > I have tested functional accesses with the ratio between functional > and timing accesses for different ratios -- 100:0, 99:1, 90:1, 50:50, > 10:90, 1:99. It is working in all the cases. > > > Thanks, > > Nilay > > _______________________________________________ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
