----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2902/#review6517 -----------------------------------------------------------
Overall, this is a patch that moves Ruby in the right direction. One overall comment: there are places where variable names are inconsistent. For instance, the local variable for the RubySystem is rs, ruby_system, and r_sys in different places below. I'm certainly not going to hold up this patch for something so trivial, but I think it's something we should keep in mind and try to aviod. Just to double check, have you compiled all of the Ruby protocols and run a simple test with them (e.g. hello)? I find that often each protocol exercises unique parts of Ruby. configs/ruby/MESI_Three_Level.py (line 99) <http://reviews.gem5.org/r/2902/#comment5599> All of these can be removed if you use Parent.any in CacheMemory.py like you do in Sequencer.py This is an important change so that all of us with external Ruby configurations don't have to make/merge these changes. src/mem/protocol/RubySlicc_Types.sm (line 160) <http://reviews.gem5.org/r/2902/#comment5591> Why is this being commented out? Was this a mistake? src/mem/ruby/profiler/AddressProfiler.hh (line 50) <http://reviews.gem5.org/r/2902/#comment5593> I don't know if it's officially part of the code guidelines, but I think you should have the variable name for the RubySystem here. src/mem/ruby/profiler/Profiler.hh (line 69) <http://reviews.gem5.org/r/2902/#comment5594> Same as above, but I guess this one was already like that. src/mem/ruby/structures/Cache.py (line 49) <http://reviews.gem5.org/r/2902/#comment5595> Here, you can use "Parant.any" and then you won't have to make all of the changes to the Python config files. This is the most important thing to change in this patch. src/mem/ruby/structures/RubyMemoryControl.cc (line 313) <http://reviews.gem5.org/r/2902/#comment5596> IMO, before this patch is committed this issue should be resolved. Either remove the debug statement, leave out the time from the printed info, or pass the ruby_system to the RubyMemoryController. The second is my prefered solution, but I don't have strong feelings on which to use. src/mem/ruby/system/Sequencer.cc (line 529) <http://reviews.gem5.org/r/2902/#comment5600> What is the purpose of this change? Functionally, it's exactly the same. Do you plan to make m_warmup_enabled not static in a future patch (which IMO we should)? Either way, a getter/setter seems like a more extensible pattern to use here. I'm not pushing for any particular change, but I would like to know the purpose of this update. Especially since this has already been a contentious piece of code (though, I don't know why). src/mem/slicc/symbols/Type.py (line 425) <http://reviews.gem5.org/r/2902/#comment5601> Again, I don't like just commenting out code that this change makes difficult. It should be solved, or the code removed. In this case, I think it's OK to remove the code, though I don't quite understand when this function is called. src/python/swig/pyobject.cc (line 111) <http://reviews.gem5.org/r/2902/#comment5597> Is it possible for a message buffer to connect two different RubySystems? This seems like a recipie for bugs in the future. Maybe assert that the two systems are the same if there is a buffer between two controllers. - Jason Power On June 19, 2015, 4:40 a.m., Brandon Potter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2902/ > ----------------------------------------------------------- > > (Updated June 19, 2015, 4:40 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10877:2e6e2150542a > --------------------------- > ruby: removes g_system_ptr and replaces with object based references > > > Diffs > ----- > > configs/ruby/MESI_Three_Level.py ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > configs/ruby/MESI_Two_Level.py ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > configs/ruby/MI_example.py ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > configs/ruby/MOESI_CMP_directory.py > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > configs/ruby/MOESI_CMP_token.py ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > configs/ruby/MOESI_hammer.py ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/protocol/RubySlicc_Types.sm > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/SConscript ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/common/Global.hh ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/common/Global.cc ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/network/MessageBuffer.hh > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/network/MessageBuffer.cc > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/network/simple/SimpleNetwork.cc > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/network/simple/Switch.cc > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/network/simple/Throttle.cc > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/profiler/AddressProfiler.hh > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/profiler/AddressProfiler.cc > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/profiler/Profiler.hh ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/profiler/Profiler.cc ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/slicc_interface/AbstractController.cc > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/structures/BankedArray.hh > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/structures/BankedArray.cc > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/structures/Cache.py ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/structures/CacheMemory.cc > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/structures/RubyMemoryControl.cc > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/structures/SConscript ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/structures/WireBuffer.cc > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/system/DMASequencer.hh > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/system/DMASequencer.cc > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/system/RubyPort.hh ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/system/RubyPort.cc ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/system/Sequencer.cc ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/system/Sequencer.py ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/system/System.hh ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/system/System.cc ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/slicc/symbols/StateMachine.py > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/slicc/symbols/Type.py ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/python/swig/pyobject.cc ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > > Diff: http://reviews.gem5.org/r/2902/diff/ > > > Testing > ------- > > > Thanks, > > Brandon Potter > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
