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