Review Board 2.0.15


ruby: add parameters to functions related to addresses

Review Request #3424 - Created April 4, 2016 and updated

Information
Brandon Potter
gem5
default
Reviewers
Default
Changeset 11426:065361f6fd53
---------------------------
ruby: add parameters to functions related to addresses

The four functions related to addresses that require knowing the block
size are printAddress, getOffset, makeNextStrideAddress, and
makeLineAddress.

The block size is added to the function signature and is passed into the
constructors in this patch.

   

Issue Summary

1 1 0 0
Posted (April 11, 2016, 2:02 p.m.)



  
src/mem/ruby/common/Address.cc (Diff revision 1)
 
 

I think the solution to cleaning up these patches is to make these functions members of RubySystem (instance functions). There's no reason these functions should be static like this.

Then, you only have to modify the places where these functions are called to use the ruby_system pointer that's already in most functions.

Maybe I'm wrong, and maybe it's actually worse this way. What do you think?

  1. If you make these functions members of the RubySystem object, you run into the issue where you need to pass the RubySystem handle to issue the invocation. RubySystem isn't available in alot of the generated code that deals with dynamically created objects (Table\<ENTRY>, DataBlk, WriteMask). These objects use the Addr functions: getOffset, makeLineAddress, etc..

    It ends up being the same problem where the RubySystem gets passed to all of these dynamically created object so that the Addr functions can be called. I agree that it's not the most beautiful solution, but IMO it's better to pass the block size instead of passing a RubySystem pointer everywhere to make the block size available.

    Let me know if you disagree; I'm open to further suggestions.

  2. In the non-Ruby memory system the classes that need to know the cache-line size either query the system, or gets the value passed to them when constructed. Perhaps it's worth doing the same for Ruby?

  3. Andreas, with this set of patches, the value is passed when the objects are constructed.

    Querying the system for the cache-lins size is difficult. The functions in this patch are not tied to any object; if you tie them to an object (as Jason suggests), you need to pass a pointer to that object to whatever wants to use the functions. The problem is that everything in Ruby wants to use these functions.

  4. ...but every Ruby component has a pointer to the RubySystem. I'm with Jason and Andreas: Handling block size and address mappings needs to be homed in a single place that all Ruby objects can access. Spreading these parameters around to all the different protocol files is be very painful and won't be very extensible. The RubySystem instance is a natural place to keep these.

  5. Joel, what do you mean by "every Ruby component"? If you mean every object that exists under the umbrella of Ruby source code, I'd argue that what you're saying is not true. Not everything has access to the RubySystem object that it's supposed to belong to. Several objects which don't have that access are Messages, Entries, DataBlks, and WriteMasks. How would you solve the issue of passing the RubySystem handle to those objects?

    If you suggest that we should pass a RubySystem to them, let me point out http://reviews.gem5.org/r/3113/. In that review, search for the phrase "transient state". You specifically say that objects that are transient (specifically referring to entries) should not have access to objects that are persistent (referring to RubySystem).

    After the initial review of 3113, I tried to address some of your comments before resigning to switch over to Nilay's method which avoided passing a handle to a "persistent object" around to "transient state". I could have done a decent job addressing some of the comments that I didn't get around to addressing, but some of the ones like this "transient state" comment were fundamentally against the whole design of what I was trying to achieve by passing RubySystem pointer around. So, I changed course on this project and adopted what Nilay had coded up. (Honestly, I felt his solution was superior in terms of elegance. He didn't modify the Types.py and StateMachine.py files as much as I had to special case everything. Instead he solved the problem with some pretty reasonable fixes.) Adopting this current version of the patches required alot of work; he originally on had this working for a couple of the public protocols with SimpleNetwork.

    If you still do not like the overall direction or design and feel that it's fundamentally flawed because certain parameters like the block size need to be centralized (like it was in my previous code), please post a solution that you're comfortable with. I will review it for you.

  6. Hi Brandon. To clarify, I've only been opposing particular aspects of these patches. I appreciate your efforts here, and I just think we need to clarify the spectra of potential solutions so we can land somewhere between all these proposed changes.

    In the 3113 review request that you point to, I define what I mean by a Ruby component: Ruby's persistent simulation objects (i.e. not the transient state). To clarify that farther, here are the core components I feel we should distribute RubySystem pointers to (if they don't already have one): the generated cache controllers (through AbstractController), cache tag/data arrays (CacheMemory, DirectoryMemory, PerfectCacheMemory, TLBTable), Sequencers and Coalescers that descend from RubyPort, network components as far as necessary (e.g. SimpleNetwork Throttles appear to need one, but Switches do not), and maybe MessageBuffers. Each of these components should have access to the common RubySystem functions/parameters (e.g. block size, address mapping functions), because each is responsible and accountable for setting up and manipulating various transient state objects (e.g. cache entries, DataBlks, NetDests, etc.). Transient state, on the other hand, is manipulated by Ruby components, so it is oblivious to the system configuration around it (i.e. doesn't make sense to have RubySystem pointers).

    So to summarize, I'm recommending a solution that lands between the extremes that we've talked about. One extreme is to distribute all parameters to all objects that need them. The other extreme is to distrubute RubySystem pointers to all objects that need them, including transient state. Both of these extremes have painful issues, but a sensible solution exists in between: Persistent Ruby objects should have a pointer to the RubySystem, and they should use the pointer to access necessary functions/parameters for manipulating their transient state (Apologies if this wasn't clear from my review of 3113).