> 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.
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.) > On Sept. 15, 2015, 2:37 p.m., Joel Hestness wrote: > > src/mem/protocol/MOESI_CMP_directory-L2cache.sm, line 737 > > <http://reviews.gem5.org/r/3113/diff/1/?file=49353#file49353line737> > > > > I don't recall where we ended on the mapping function discussion. > > However, mapping functions will be specific to each RubySystem instance. I > > believe we had the preference that these calls end up looking like: > > > > ruby_system.map_Address_to_Directory(address) This function is a semi-wrapper for another function "map_Address_to_DirectoryNode()" which needs to be referenced with the RubySystem pointer. The file that this is generated in is "RubySlicc_ComponentMapping.hh". I'm arguing that this needs to be a parameter to be able to dereference in that function. Also, the change seems somewhat superficial in that they both achieve the same thing. > On Sept. 15, 2015, 2:37 p.m., Joel Hestness wrote: > > src/mem/protocol/RubySlicc_ComponentMapping.sm, line 32 > > <http://reviews.gem5.org/r/3113/diff/1/?file=49365#file49365line32> > > > > Almost all of these functions are going to need to be RubySystem > > instance specific... How do I go about doing this? These are generated by Slicc from within Types.py and are specific to each protocol. Blindly moving these functions directly into the RubySystem class does not work (obviously). Since this involves re-examining previous assumptions about whether these functions are globally visibile, I'd like to avoid doing this unless you have something clever up your sleeve. To me, the cure seems worse than the disease. > On Sept. 15, 2015, 2:37 p.m., Joel Hestness wrote: > > src/mem/protocol/MOESI_CMP_directory-L2cache.sm, line 1518 > > <http://reviews.gem5.org/r/3113/diff/1/?file=49353#file49353line1518> > > > > This doesn't make sense. Cache entries should not need a pointer to the > > RubySystem. Certainly the caches have RubySystem pointers, right? The entries do not have a pointer to the object which owns them and I am not aware of a way to back-reference up the object hierarchy for these "transient" objects. (For instance, look at PersistentTable.hh; there's not a way with the current code to reference out of the entry to obtain the RubySystem pointer.) The main issue with these objects needing the pointer is to allow them to use the pointer from with their NetDest objects. It may be possible to circumvent NetDest needing the RubySystem pointer, but it's unclear to me how. > On Sept. 15, 2015, 2:37 p.m., Joel Hestness wrote: > > src/mem/protocol/RubySlicc_Types.sm, line 72 > > <http://reviews.gem5.org/r/3113/diff/1/?file=49368#file49368line72> > > > > Transient state shouldn't have pointers to persistent objects. I am open to suggestions if the modifications are reasonable. > On Sept. 15, 2015, 2:37 p.m., Joel Hestness wrote: > > src/mem/ruby/structures/DirectoryMemory.cc, line 50 > > <http://reviews.gem5.org/r/3113/diff/1/?file=49420#file49420line50> > > > > Needs to be set in a different way with more appropriate abstraction > > (i.e. m_numa_high_bit should be private in RubySystem, and set once not by > > components outside RubySystem). I removed the directories and the bits field which is derived from it. I've tried the different protocols and I don't see where more than one directory is used in the public protocols so the m_numa_high_bit doesn't seem useful. Maybe I am missing something obvious, but it doesn't seem like this field is useful at least in the public code? > On Sept. 15, 2015, 2:37 p.m., Joel Hestness wrote: > > src/mem/slicc/symbols/Type.py, line 521 > > <http://reviews.gem5.org/r/3113/diff/1/?file=49452#file49452line521> > > > > These functions need to be moved into the RubySystem, since they are > > properties of each system instance. As evidence, in line 736, you're just > > calling a function of the RubySystem anyway. I am willing to make this change if you explicitly explain how you'd like this done. I've spent a bit of time hammering on this code and I haven't come up with anything cleaner than what I have. - Brandon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3113/#review7177 ----------------------------------------------------------- On Oct. 28, 2015, 11:37 p.m., Brandon Potter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3113/ > ----------------------------------------------------------- > > (Updated Oct. 28, 2015, 11:37 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/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 > 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/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/BasicRouter.hh > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/network/BasicRouter.cc > 3a4d1b5cd05ceed30dd0f5341fac9bbe41a193d6 > src/mem/ruby/network/BasicRouter.py > 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 > 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 > 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 > > 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 gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
