> On Sept. 15, 2015, 2:37 p.m., Joel Hestness wrote: > > This patch really needs to be broken into smaller chunks. It contains > > numerous changes without sufficient description, which will make it very > > difficult to merge with. In general, these changes need better > > object-oriented design. Also, Reviewboard won't load/display changes to > > RubySystem.cc, which is pretty important for reviewing this patch. > > > > I recommend splitting out the following as separate patches: > > 1) Move controller, directory, etc. variables into RubySystem, add > > accessors as appropriate, remove static designators as appropriate, add > > const designators as appropriate and initialize. > > 2) Move testAndRead and testAndWrite into DataBlk. > > 3) Distribute RubySystem pointers. To do this, introduce a heritable class > > (let's call it 'RubySystemComponent' for the purpose of discussion), which > > contains a RubySystem pointer and some miscellaneous functions (Feel free > > to ping me if you need more detail on how to do this. I feel pretty > > strongly that we should do this as described here): > > A) Inherit from RubySystemComponent in all persistent simulation objects > > (simulated system components, but not state) that need a RubySystem pointer > > (e.g. CacheMemory, DirectoryMemory, PerfectCacheMemory, Network, > > NetworkInterface, etc.). This inherited type will at least signal how the > > RubySystem pointers should be distributed as compared to this patch, which > > just uses them wherever to get things working. Further, this will unify the > > naming of the RubySystem pointer, so that every component will reference it > > with the same variable name for consistency. > > B) Make machineCount(), makeLineAddress(), mapAddressToRange(), and > > map_Address_to_Directory() (etc.?) functions of both the RubySystem and > > RubySystemComponent. This will eliminate the need to pass a RubySystem > > pointer or call these functions on a RubySystem pointer by encapsulating > > their functionality in the inherited class functions. In > > RubySystemComponent, call the appropriate RubySystem function through the > > RubySystem pointer. > > C) Make MachineType_base* functions of RubySystem and > > RubySystemComponent. NOTE: This patch currently appears to make it so that > > multiple RubySystem instances must share a common coherence protocol. > > Eventually, a RubySystem instance might track the protocol it is > > simulating, so the MachineType_base* functions would need to be > > protocol-dependent. These can still be generated code but implement > > RubySystem/RubySystemComponent functions. > > D) Avoid getRubySystem() functions. In a few cases, the presence of > > these appear to signal that a component is missing a RubySystem pointer > > when it should have one. In other cases, child classes (e.g. ports within > > other components) can have direct access to the pointer in the parent > > class, since there is sufficient encapsulation/abstraction of who owns and > > can access the pointer. > > > > I've provided lots of comments below, but I would strongly prefer that this > > patch be broken into smaller chunks. > > Brandon Potter wrote: > Hey Joel, thanks for the feedback! I had hoped that you would respond to > this review. I'll take a look at the issues below this weekend. By breaking > the patch into pieces, it's likely that they won't compile and run > individually. If I break them apart, do we agree that they should be > recombined before pushing as a changeset? > > Regarding A, I had originally coded a version of "RubySystemComponent" > that held the ruby system pointer, but not much else. So, the addition of the > class didn't seem to add anything worthwhile. If I can move the functions in > B into the class too, it seems like it could be feasible. The difficulty here > is generating the pointer dereference in slicc through the object. As I > remember, it was pretty difficult to do so. I'd appreciate a bit of help here > if this is what you really want. > > For C, I might be able to move the MachineType_base functions, but I > din't have any intention of making the RubySystem instance generation > protocol-independent. That might be something that I could try in the future, > but not near-term. > > On D, I noticed this was added to the code and there didn't appear to be > a good reason other than encapsulation. I'm not sure that it's even > necessary. I can take a crack at it. > > Joel Hestness wrote: > Cool. I appreciate the willingness to iterate on these changes. > > > By breaking the patch into pieces, it's likely that they won't compile > and run individually. If I break them apart, do we agree that they should be > recombined before pushing as a changeset? > > The numbered items above can certainly be implemented, compiled, and run > as separate patches. I expect that the lettered items under (3) can even be > broken into separate changes if it makes sense to do them incrementally. > Splitting out the changes from this patch into separate patches/reviews will > make it much easier for someone merging their code to see why things are > changing the way they are, so I expect many of us would prefer they be pushed > as separate changes. > > > Regarding A, I had originally coded a version of "RubySystemComponent" > that held the ruby system pointer, but not much else. So, the addition of the > class didn't seem to add anything worthwhile. If I can move the functions in > B into the class too, it seems like it could be feasible. The difficulty here > is generating the pointer dereference in slicc through the object. As I > remember, it was pretty difficult to do so. I'd appreciate a bit of help here > if this is what you really want. > > I might be wrong, but I don't think any of the functions I listed in (B) > will require modifying SLICC. I'd recommend trying to move these functions > incrementally as long as they don't require SLICC modification. Save > functions with potential SLICC mods for (C). If you get a patch ready that > adds the 'RubySystemComponent', feel free to email it to me, and we can > discuss more concrete ways to migrate functions. > > > For C, I might be able to move the MachineType_base functions, but I > din't have any intention of making the RubySystem instance generation > protocol-independent. That might be something that I could try in the future, > but not near-term. > > Sure, I was just noting that the RubySystem may eventually need to track > the coherence protocol it is simulating. This is another indication that it > makes sense for the RubySystem to continue to exist (in addition to various > object hierarchy analogs like SimObjects/EventManagers, Serializable, etc.). > > I suspect the trickier thing with functions listed in (C) will be > generating necessary code with SLICC. Again, I'd be happy to help figure out > how to make that work - just keep me posted. > > Brandon Potter wrote: > Hello Joel, > > I have not addressed everything in the way that you'd asked, but I feel > that this iteration is a substantial improvement over the last. I am really > torn on implementing the last suggestion. Would you mind reviewing the code > again and we can discuss it further if you're still unhappy? (I've tried to > compromise by including Nilay's work on the block addresses and in retrospect > I think it's a substantial improvement.)
Thank you for tackling some of this. I do feel like we still have a ways to go on this. It appears that many of the issues you have left are because we have yet to introduce the RubySystemComponent abstraction. With that in place, I expect challenges with machine type functions, and removal of RubySystem pointers from transient objects will be overcome pretty smoothly. We had an email exchange RE: NetDests, but I didn't receive a response on that thread, and it looks like you're still facing those challenges in the latest patch. Perhaps we can reconvene in that email thread or via phone? Please let me know how you'd prefer to proceed. - Joel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3113/#review7177 ----------------------------------------------------------- On Nov. 2, 2015, 7:11 p.m., Brandon Potter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3113/ > ----------------------------------------------------------- > > (Updated Nov. 2, 2015, 7:11 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11093:3c32e2cb1c87 > --------------------------- > ruby: allows multiple instances of ruby to be invoked > > This patch removes the restriction that there can only be a single ruby > system object in a given simulation. With the configuration supplied, it is > possible to run an instance of ruby with any of the supplied topologies. > > The problem centers around the use of globals and static class members in the > ruby source. Conceptually, this prohibits multi-instance simulations > since the ruby systems end up sharing values which should be distinct. (The > earliest manifestation of this problem occurs when important runtime > components use shared variables for MachineType sanity checks which will > trigger failures.) > > The idea behind the patch is to move as many as the statics/globals as > necessary into the ruby system object. The ruby system object acts as the > single point of reference for the child objects which needed the > globals/statics. With multi-instance runs, each ruby system object will > contain distinct values for the previous globals/statics. Unfortunately, this > requires passing the ruby system pointer throughout the object hierarchy and > may incur some minor performance overhead due to extra indirect references > through the ruby system object. > > > Diffs > ----- > > src/mem/ruby/structures/DirectoryMemory.hh > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/structures/DirectoryMemory.cc > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/structures/DirectoryMemory.py > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/structures/PerfectCacheMemory.hh > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/structures/PersistentTable.hh > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/structures/PersistentTable.cc > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/structures/Prefetcher.hh > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/structures/Prefetcher.cc > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/structures/RubyMemoryControl.hh > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/structures/RubyMemoryControl.cc > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/structures/RubyMemoryControl.py > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/structures/RubyPrefetcher.py > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/structures/TBETable.hh > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/structures/TimerTable.cc > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/structures/WireBuffer.hh > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/structures/WireBuffer.py > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/system/CacheRecorder.cc > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/system/DMASequencer.hh > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/system/DMASequencer.cc > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/system/RubyPort.hh 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/system/RubyPort.cc 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/system/RubySystem.hh 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/system/RubySystem.cc 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/system/Sequencer.hh 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/system/Sequencer.cc 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/slicc/ast/EnqueueStatementAST.py > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/slicc/ast/NewExprAST.py 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/slicc/ast/ObjDeclAST.py 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/slicc/parser.py 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/slicc/symbols/StateMachine.py > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/slicc/symbols/Type.py 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > configs/example/multi-system-se.py PRE-CREATION > configs/ruby/Ruby.py 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/cpu/testers/rubytest/RubyTester.hh > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/cpu/testers/rubytest/RubyTester.cc > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/cpu/testers/rubytest/RubyTester.py > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/protocol/MESI_Three_Level-L0cache.sm > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/protocol/MESI_Three_Level-L1cache.sm > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/protocol/MESI_Two_Level-L1cache.sm > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/protocol/MESI_Two_Level-L2cache.sm > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/protocol/MESI_Two_Level-dir.sm > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/protocol/MESI_Two_Level-dma.sm > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/protocol/MI_example-cache.sm > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/protocol/MI_example-dir.sm 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/protocol/MI_example-dma.sm 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/protocol/MOESI_CMP_directory-L1cache.sm > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/protocol/MOESI_CMP_directory-L2cache.sm > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/protocol/MOESI_CMP_directory-dir.sm > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/protocol/MOESI_CMP_directory-dma.sm > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/protocol/MOESI_CMP_token-L1cache.sm > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/protocol/MOESI_CMP_token-L2cache.sm > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/protocol/MOESI_CMP_token-dir.sm > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/protocol/MOESI_CMP_token-dma.sm > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/protocol/MOESI_hammer-cache.sm > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/protocol/MOESI_hammer-dir.sm > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/protocol/MOESI_hammer-dma.sm > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/protocol/Network_test-cache.sm > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/protocol/RubySlicc_ComponentMapping.sm > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/protocol/RubySlicc_Defines.sm > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/protocol/RubySlicc_Exports.sm > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/protocol/RubySlicc_Types.sm > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/common/DataBlock.cc 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/common/NetDest.hh 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/common/NetDest.cc 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/filters/BulkBloomFilter.cc > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/network/MessageBuffer.hh > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/network/MessageBuffer.cc > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/network/MessageBuffer.py > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/network/Network.hh 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/network/Network.cc 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/network/Network.py 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/network/Topology.hh 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/network/Topology.cc 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/network/garnet/fixed-pipeline/NetworkInterface_d.cc > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/network/garnet/flexible-pipeline/NetworkInterface.cc > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/network/simple/Throttle.hh > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/network/simple/Throttle.cc > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/profiler/Profiler.hh 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/profiler/Profiler.cc 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/slicc_interface/AbstractController.hh > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/slicc_interface/AbstractController.cc > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/slicc_interface/Controller.py > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/slicc_interface/Message.hh > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/slicc_interface/RubyRequest.hh > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/slicc_interface/RubySlicc_ComponentMapping.hh > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/structures/BankedArray.hh > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/structures/BankedArray.cc > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/structures/CacheMemory.hh > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/structures/CacheMemory.cc > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > > Diff: http://reviews.gem5.org/r/3113/diff/ > > > Testing > ------- > > gem5/util/regress (to check builds) and custom tests (which use the new > configuration) > > > Thanks, > > Brandon Potter > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
