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 (Diff revision 1)

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.

Show all issues

1. Brandon Potter June 22, 2015, 1:54 p.m. (June 22, 2015, 1:54 p.m.)

   Hi Jason, I am testing the code and it doesn't appear to work with the changes that were suggested.
   "fatal: system.ruby.l1_cntrl0.L1Dcache.ruby_system without default or user set value"
   I am not entirely sure why it doesn't work; best guess (current hypothesis) is that the object hierarchy is not established when the parameters are created with Python.  The other objects in the configuration files require explicitly passing the RubySystem pointer so it appears that this might be necessary.

2. Jason Lowe-Power June 22, 2015, 2:06 p.m. (June 22, 2015, 2:06 p.m.)

   I was going to look at this, but I can't get this version of the patch to apply cleanly to the current gem5 mainline. I'm not sure what's wrong...

3. Joel Hestness June 22, 2015, 6:45 p.m. (June 22, 2015, 6:45 p.m.)

   Parent.any looks for the same named variable in the parent SimObject's parameters. This won't work for these controllers, because the pointer you're looking for IS the ruby_system (which doesn't have a pointer to itself). You might be able to just default the parameter to Parent.

4. Brandon Potter June 22, 2015, 8:19 p.m. (June 22, 2015, 8:19 p.m.)

   I do believe that I have this working now with what Jason suggested.  Not sure what problem I was encountering earlier, but I seem to have managed to get it working with all of the protocols.

src/mem/protocol/RubySlicc_Types.sm (Diff revision 1)

Why is this being commented out? Was this a mistake?

Show all issues

1. Brandon Potter June 22, 2015, 12:19 p.m. (June 22, 2015, 12:19 p.m.)

   This is another instance where it was difficult to pass the ruby system pointer.  I will revisit all of these and try to figure out how to handle them.

src/mem/ruby/profiler/AddressProfiler.hh (Diff revision 1)

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 (Diff revision 1)

Same as above, but I guess this one was already like that.

Show all issues

src/mem/ruby/structures/Cache.py (Diff revision 1)

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.

Show all issues

src/mem/ruby/structures/RubyMemoryControl.cc (Diff revision 1)

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.

Show all issues

1. Brandon Potter June 26, 2015, 10:29 a.m. (June 26, 2015, 10:29 a.m.)

   RubyMemoryControl is deprecated, right?  Advocating to remove the DPRINTF to prevent needing to figure out how to pass a pointer to RubyMemoryControl.

src/mem/ruby/system/Sequencer.cc (Diff revision 1)

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).

Show all issues

1. Brandon Potter June 22, 2015, 12:19 p.m. (June 22, 2015, 12:19 p.m.)

   I do not have a strong opinion either way; agree that it functionally does the same thing.  My code predated the modification by Joel. When I merged my changes against the mainline, I took my solution over his (since it gets rid of the static variable).  Not arguing that this is better because it does something different, but it is more consistent with what is done in my other patches.
   Joel, if you have an opinion on this let me know and I will address it.

2. Jason Lowe-Power June 22, 2015, 12:27 p.m. (June 22, 2015, 12:27 p.m.)

   Did you mean to make m_warmup_enabled not static? In src/mem/ruby/system/System.hh on line 141 (new version) it's still static.
   I'm not against what you have here, and I'm also for making it not static (in this patch or not). Again, I don't really care, just wanted to make sure something doesn't get overlooked.

src/mem/slicc/symbols/Type.py (Diff revision 1)

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.

Show all issues

1. Brandon Potter June 26, 2015, 10:29 a.m. (June 26, 2015, 10:29 a.m.)

   Advocating for removing the code, because it's not clear to me on how to pass a pointer in here without passing in a pointer to all of the objects that use this "::print" function.  It only wants to print for "message" types, but it would require making the pointer available to all types to do so.  I think doing that would be painful and unnecessary to support a print message.

src/python/swig/pyobject.cc (Diff revision 1)

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.

Show all issues

Can you please expand on the commit message? I understand that these changes are to allow multiple RubySystem instances, but some detailed information about this should be available in the commit log.

src/mem/ruby/system/Sequencer.cc (Diff revision 1)

Currently, all references to warm-up and cool-down variables should use the static accessors, RubySystem::getWarmupEnabled() and RubySystem::getCooldownEnabled(), respectively.
I stand by my observation that warm-up and cool-down are simulation-wide operations, not per-RubySystem instance. This is because controller activity from one RubySystem instance may affect controllers in other RubySystems (e.g. cache flushing during cool-down). m_warmup_enabled and m_cooldown_enabled should remain static variables of the RubySystem class.
Is there something getting in the way of leaving these static?

Show all issues

1. Steve Reinhardt June 22, 2015, 7:26 p.m. (June 22, 2015, 7:26 p.m.)

   I'm confused---can two Ruby components even talk to each other if they're not part of the same RubySystem?
   I haven't looked at the code closely, so I don't have a strong opinion, but I'll say that:
   
   1. It does seem unlikely that you would be doing warmup or cooldown in one RubySystem but not in another, but given that we've never run a simulation with two RubySystem objects before, it also seems premature to declare that we'd never want to do that.
   
   2. If making the warmup/cooldown flags static actually eliminates the need for having a RubySystem pointer in a significant number of objects, then it might be worthwhile, but if that pointer needs to be in those objects for other reasons anyway, then it doesn't seem like much of a win.

2. Joel Hestness June 23, 2015, 7:41 a.m. (June 23, 2015, 7:41 a.m.)

   
   
   
   can two Ruby components even talk to each other if they're not part of the same RubySystem?
   
   This is the important question. I'm not sure how multiple RubySystem instances will be used. However, it is worth noting that for warm-up and cool-down, Ruby sends memory accesses through the sequencers and RubySystem instance checkpoints will contain traces accessing a particular subset of sequencers. Thus, RubySystem instances in a single simulation should maintain disjoint sets of sequencers. Beyond that, however, it's not clear whether the controllers of separate instances may interact during warm-up and cool-down. This seems likely if the separate RubySystem instances were to represent, for example, separate but coherent sockets in the simulated system. It is definitely possible that separate instances can sink their cool-down flushes to the same memory, and warm-up accesses could access the same memory.
   Given how complicated this restructuring is and that we're not completely clear how multiple instances may interact, I think it will be worth the effort to maintain these variables as static and use the accessor functions until/unless we find a situation where they need to be changed.
   Also (a question for Brad maybe?), does anyone know why we don't do RubySystem cool-down during drain(), and why we don't do warm-up during startup() like other gem5 components?

src/mem/ruby/network/MessageBuffer.cc (Diff revision 2)

Per my other post: This change should not occur here.

Show all issues

src/mem/ruby/system/Sequencer.cc (Diff revision 2)

This should still use the accessor getWarmupEnabled()

Show all issues

src/mem/ruby/system/Sequencer.cc (Diff revision 2)

Same: Use accessor

Show all issues

src/mem/ruby/system/Sequencer.cc (Diff revision 2)

Same: Use accessor

Show all issues

src/mem/ruby/system/System.hh (Diff revision 2)

These should not be public. Access them using the accessor.

Show all issues

src/mem/ruby/system/System.cc (Diff revision 2)

These are already initialized statically at the head of this file. These accesses should not occur here.

Show all issues

src/python/swig/pyobject.cc (Diff revision 2)

the include order seems off

Show all issues

Thanks for all the updates. Other than the minor thing below it looks ready to go.

src/mem/ruby/system/Sequencer.cc (Diff revision 3)

Typo? Shouldn't this be a function call? I could be missing something, though.

Show all issues

1. Brandon Potter June 26, 2015, 11:25 a.m. (June 26, 2015, 11:25 a.m.)

   Nice catch, thanks!

Brandon, overall I would suggest, if it is possible, that you spend sometime on making

RubySystem a sim object.  That way you would not require to copy the pointer to so 

many places.  I think objects that need clock really should be clocked objects

themselves or hold pointer to some real clocked object.  RubySystem is really a glue

object which should not have a clock of its own.

src/mem/ruby/network/MessageBuffer.hh (Diff revision 3)

Why is RubySystem required here?  If you can, move it to MessageBuffer.cc.

Show all issues

src/mem/ruby/profiler/AddressProfiler.hh (Diff revision 3)

??

Show all issues

src/mem/ruby/profiler/Profiler.cc (Diff revision 3)

OK.  Pass this pointer here and in AddressProfiler.cc, directly make the function call on the Profiler.

Show all issues

src/mem/ruby/structures/BankedArray.cc (Diff revision 3)

I strongly suggest that you write a patch that is applied before this ruby system patch.  The new patch would do away with this tick handling.  Everything should be handled in Cycles.  The function tryAccess() would receive a current cycle time value as input, which means that curTick() would not be called.  When CacheMemory object calls tryAccess(), either CacheMemory should itself receive a current cycle value as input, or CacheMemory itself should be converted to a ClockedObject.  Ultimately the BankedArray class should not require ruby system at all.

Show all issues

1. Brandon Potter July 6, 2015, 1:40 p.m. (July 6, 2015, 1:40 p.m.)

   Hi Nilay, thanks for the review.  The other issues were clear errors and the address profiler catch was particularly good.
   It looks like other code in Ruby relies on tick calculations instead of working directly with cycles.  The start_cycle field (which has been moved into RubySystem) also relies on RubySystem being a clocked object (it was previously a global field).  So, I expect that what you are suggesting that I do will not be trivial.  Would also argue that it's only indirectly related to this patch in that the patch exposes it as a problem (where people were previously taking the global pointers for granted).  I am not disagreeing that it is a problem or that it couldn't use a bit of a cleanup, but it's beyond the scope of what I am trying to do here to enable multi-instance Ruby support.
   For clarity, what did you have in mind for ruby components that are not clocked objects, but rely on the ruby system to provide their clock (directly or indirectly)?  I could make them clocked objects and pass in their clock in the configuration files explicitly but that seems like a headache.  Did you have a cleaner solution in mind?

src/mem/ruby/structures/Cache.py (Diff revision 3)

This should not be done.  Please write a patch that either makes CacheMemory into a ClockedObject or get the time value from its controller.

Show all issues

src/mem/slicc/symbols/StateMachine.py (Diff revision 3)

Really! How come Brad and Steve not chide you for this?

Show all issues

1. Brandon Potter July 6, 2015, 1:40 p.m. (July 6, 2015, 1:40 p.m.)

   They did and I fixed it! :P