> 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.
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. - Brandon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3113/#review7177 ----------------------------------------------------------- On Sept. 14, 2015, 11:58 p.m., Brandon Potter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3113/ > ----------------------------------------------------------- > > (Updated Sept. 14, 2015, 11:58 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/protocol/MESI_Two_Level-L1cache.sm > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > configs/common/Options.py 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > configs/example/multi-system-se.py PRE-CREATION > configs/ruby/Ruby.py 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/cpu/testers/rubytest/Check.cc 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/cpu/testers/rubytest/RubyTester.hh > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/cpu/testers/rubytest/RubyTester.cc > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/cpu/testers/rubytest/RubyTester.py > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/protocol/MESI_Three_Level-L0cache.sm > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/protocol/MESI_Three_Level-L1cache.sm > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/slicc/parser.py 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/slicc/symbols/StateMachine.py > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/slicc/symbols/Type.py 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/python/swig/pyobject.cc 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/system/RubySystem.cc 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/system/Sequencer.hh 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/structures/WireBuffer.py > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/system/CacheRecorder.hh > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/system/CacheRecorder.cc > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/system/DMASequencer.cc > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/system/RubyPort.hh 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/system/RubyPort.cc 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/system/RubySystem.hh PRE-CREATION > src/mem/ruby/structures/TBETable.hh > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/structures/TimerTable.hh > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/structures/TimerTable.cc > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/structures/WireBuffer.hh > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/protocol/MESI_Two_Level-L2cache.sm > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/protocol/MESI_Two_Level-dir.sm > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/protocol/MESI_Two_Level-dma.sm > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/protocol/MI_example-cache.sm > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/protocol/MI_example-dir.sm 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/protocol/MI_example-dma.sm 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/protocol/MOESI_CMP_directory-L1cache.sm > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/protocol/MOESI_CMP_directory-L2cache.sm > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/protocol/MOESI_CMP_directory-dir.sm > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/protocol/MOESI_CMP_directory-dma.sm > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/protocol/MOESI_CMP_token-L1cache.sm > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/protocol/MOESI_CMP_token-L2cache.sm > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/protocol/MOESI_CMP_token-dir.sm > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/protocol/MOESI_CMP_token-dma.sm > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/protocol/MOESI_hammer-cache.sm > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/protocol/MOESI_hammer-dir.sm > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/protocol/MOESI_hammer-dma.sm > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/protocol/Network_test-cache.sm > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/protocol/Network_test-dir.sm > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/protocol/RubySlicc_ComponentMapping.sm > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/protocol/RubySlicc_Defines.sm > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/protocol/RubySlicc_Exports.sm > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/protocol/RubySlicc_Types.sm > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/protocol/RubySlicc_Util.sm 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/SConscript 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/common/Address.hh 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/common/Address.cc 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/common/DataBlock.hh 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/common/DataBlock.cc 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/common/NetDest.hh 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/common/NetDest.cc 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/common/SubBlock.hh 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/common/SubBlock.cc 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/filters/AbstractBloomFilter.hh > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/filters/BlockBloomFilter.hh > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/filters/BlockBloomFilter.cc > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/filters/BulkBloomFilter.hh > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/filters/BulkBloomFilter.cc > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/filters/H3BloomFilter.cc > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/filters/LSB_CountingBloomFilter.cc > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/filters/MultiBitSelBloomFilter.cc > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/filters/MultiGrainBloomFilter.cc > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/filters/NonCountingBloomFilter.cc > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/network/BasicLink.hh 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/network/BasicLink.cc 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/network/BasicLink.py 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/network/BasicRouter.hh > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/network/BasicRouter.cc > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/network/BasicRouter.py > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/network/MessageBuffer.hh > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/network/MessageBuffer.cc > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/network/MessageBuffer.py > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/network/Network.hh 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/network/Network.cc 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/network/Network.py 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/network/Topology.hh 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/network/Topology.cc 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/network/fault_model/FaultModel.hh > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/network/fault_model/FaultModel.cc > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/network/fault_model/FaultModel.py > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/network/garnet/fixed-pipeline/NetworkInterface_d.cc > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/network/garnet/flexible-pipeline/NetworkInterface.cc > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/profiler/AddressProfiler.cc > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/profiler/Profiler.hh 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/slicc_interface/AbstractController.hh > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/slicc_interface/AbstractController.cc > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/slicc_interface/Controller.py > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/slicc_interface/Message.hh > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/slicc_interface/RubyRequest.hh > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/slicc_interface/RubySlicc_ComponentMapping.hh > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/slicc_interface/RubySlicc_Util.hh > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/structures/CacheMemory.hh > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/structures/CacheMemory.cc > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/structures/DirectoryMemory.hh > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/structures/DirectoryMemory.cc > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/structures/DirectoryMemory.py > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/structures/PerfectCacheMemory.hh > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/structures/PersistentTable.hh > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/structures/PersistentTable.cc > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/structures/Prefetcher.hh > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/structures/Prefetcher.cc > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/structures/RubyCache.py > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/structures/RubyMemoryControl.hh > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/structures/RubyMemoryControl.cc > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/structures/RubyMemoryControl.py > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/structures/RubyPrefetcher.py > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/system/Sequencer.cc 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/ruby/system/Sequencer.py 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/slicc/ast/EnqueueStatementAST.py > 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/slicc/ast/NewExprAST.py 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > src/mem/slicc/ast/ObjDeclAST.py 62e1504b9c6413aa03a2f7d7eee88c8ea45cfe00 > > 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
