> On April 11, 2016, 9:02 p.m., Jason Lowe-Power wrote: > > src/mem/ruby/common/Address.cc, line 107 > > <http://reviews.gem5.org/r/3424/diff/1/?file=54449#file54449line107> > > > > 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? > > Brandon Potter wrote: > 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. > > Andreas Hansson wrote: > 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? > > Brandon Potter wrote: > 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. > > Joel Hestness wrote: > ...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. > > Brandon Potter wrote: > 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.
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). - Joel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3424/#review8180 ----------------------------------------------------------- On April 4, 2016, 11:40 p.m., Brandon Potter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3424/ > ----------------------------------------------------------- > > (Updated April 4, 2016, 11:40 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > 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. > > > Diffs > ----- > > src/mem/ruby/structures/TBETable.hh > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/ruby/structures/TimerTable.cc > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/ruby/system/DMASequencer.cc > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/ruby/system/GPUCoalescer.cc > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/ruby/system/RubyPort.cc cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/ruby/system/RubySystem.cc cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/ruby/system/Sequencer.cc cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/slicc/symbols/StateMachine.py > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/slicc/symbols/Type.py cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/ruby/slicc_interface/RubySlicc_Util.hh > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/ruby/structures/CacheMemory.cc > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/ruby/structures/PerfectCacheMemory.hh > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/ruby/structures/PersistentTable.cc > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/ruby/structures/Prefetcher.cc > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MOESI_CMP_directory-dma.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MOESI_CMP_token-dir.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/RubySlicc_Util.sm cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/ruby/common/Address.hh cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/ruby/common/Address.cc cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/ruby/filters/H3BloomFilter.cc > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/ruby/filters/MultiBitSelBloomFilter.cc > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/ruby/network/MessageBuffer.cc > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/ruby/profiler/AddressProfiler.cc > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/ruby/slicc_interface/AbstractController.cc > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/ruby/slicc_interface/RubyRequest.hh > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/cpu/testers/rubytest/RubyTester.hh > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MESI_Two_Level-dir.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MESI_Two_Level-dma.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MOESI_AMD_Base-RegionBuffer.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MOESI_AMD_Base-RegionDir.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/mem/protocol/MOESI_CMP_directory-dir.sm > cfad34a15729e1d5e096245f5a80ded6e2c379ca > src/cpu/testers/rubytest/Check.cc cfad34a15729e1d5e096245f5a80ded6e2c379ca > > Diff: http://reviews.gem5.org/r/3424/diff/ > > > Testing > ------- > > > Thanks, > > Brandon Potter > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
