> On Sept. 24, 2015, 11:32 p.m., Brad Beckmann wrote: > > Do not check this in. I suggest you stop going in this direction. Any > > performance benefits provided by this patch do not justify the resulting > > duplicate code and downstream re-correlation and development efforts. > > There are very good reasons for the separation of RubyPort and the > > sequencers. When we post our GPU patches, we post more child objects of > > RubyPort that are GPU specific. > > > > RubyPort encapsulates the generic interfaces needed to connect m5ports with > > Ruby controllers, while the child objects contain the device specific logic > > that wants to be shared across multiple protocols. For instance, the > > Sequencer contains logic for handling CPU synchronization and coalescing > > that assumes a RFO coherence protocol. The DMA sequencer is a simple > > interface that handles large, multi-line access. We will be posting the > > GPU coalescer that assumes weaker GPU coherence and coalesces memory > > requests across a wavefront. > > Nilay Vaish wrote: > I'll retain RubyPort so that you can inherit from it. The code for ports > has to be written separately for > each type of controller, as is the case all over gem5. DMA and CPU > sequencers do not share RubyPort, only CPU > sequencer inherits from RubyPort. > > Brad Beckmann wrote: > Please do not change the Sequencer and replace it with the > FirstLevelController and please do not replace the DMASequencer with the > AbstractDMAController. A lot of code exists that depends on the current > interfaces. Also please do not make any changes to the sm files until after > we post our GPU patches. In general, slow down proposing huge changes like > this. A lot of people depend on the current code. If we were even to agree > to these types of changes (and so far we do not), changes like this one need > to be made slowly. > > Also from now on, if you claim a patch improves simulation performance, > you need provide absolute numbers. Many of us are not convinced the > performance benefit justifies the development cost. > > Finally, *if* (and I emphasize if) we were to make a change like this, I > suggest we focus on eliminating the m5port->mandatory queue two-step process > for communicating requests to the L1 cache logic. Currently this patch > merges the objects, but it still relies on the two-step communication process > within the merged object. Perhaps eliminating that unecessary timing is > where you are planning to go with this, but it is not clear. If that is your > intention, please communicate the details of your plan before posting more > patches.
By the way, the DMA sequencers used to inherit from RubyPort, but late last year you changed that. That is unfortunate because now DMASequencer and RubyPort duplicate a lot of code. For instance, their MemSlavePort::recvTimingReq look almost identical, but not quite. Thus current Ruby dma devices cannot issue PIO requests. This is disappointing and right now is causing us to do more development to work around it. Given our current problems with the DMA sequencer not inheriting from RubyPort, I am very leery of allowing any more of these types of changes. Instead, I'd like to see us revert 10518:30e3715c9405. - Brad ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3134/#review7279 ----------------------------------------------------------- On Sept. 24, 2015, 2:57 p.m., Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3134/ > ----------------------------------------------------------- > > (Updated Sept. 24, 2015, 2:57 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11139:6433ce0b920e > --------------------------- > ruby: combine RubyPort and Sequencer and rename as FirstLevelController > > This patch move the file RubyPort.* to FirstLevelController.*. Then, it > copies > the contents of Sequencer.* to FirstLevelController.*. Finally, it renames > RubyPort and Sequencer to FirstLevelController and adjusts line lengths where > ever execeeded. There are two reasons for this change: > > * I find this FirstLevelController abstraction better than having separate > RubyPort and Sequencer. With the FirstLevelController class, we are trying to > imply the functionality is common to all controllers that are directly > attached > to the cores. Earlier, it appeared as if there are two components between the > core and the actual first level controller. Now the first level controllers > in > slicc would inherit from this new FirstLevelController class and we would > directly connect the controller to the core, instead of going through the > sequencer. > > * The combined entity would allow us to make some changes that would > ultimately > improve ruby's performance significantly. > > > Diffs > ----- > > src/mem/protocol/MESI_Two_Level-L1cache.sm 5d38dc2f7d66 > src/mem/protocol/MESI_Two_Level-dma.sm 5d38dc2f7d66 > src/mem/protocol/MI_example-cache.sm 5d38dc2f7d66 > src/mem/protocol/MI_example-dma.sm 5d38dc2f7d66 > src/mem/protocol/MOESI_CMP_directory-L1cache.sm 5d38dc2f7d66 > src/mem/protocol/MOESI_CMP_directory-dma.sm 5d38dc2f7d66 > src/mem/protocol/MOESI_CMP_token-L1cache.sm 5d38dc2f7d66 > src/mem/protocol/MOESI_CMP_token-dma.sm 5d38dc2f7d66 > src/mem/protocol/MOESI_hammer-cache.sm 5d38dc2f7d66 > src/mem/protocol/MOESI_hammer-dma.sm 5d38dc2f7d66 > src/mem/protocol/Network_test-cache.sm 5d38dc2f7d66 > src/mem/protocol/RubySlicc_Exports.sm 5d38dc2f7d66 > src/mem/protocol/RubySlicc_Types.sm 5d38dc2f7d66 > src/mem/ruby/SConscript 5d38dc2f7d66 > src/mem/ruby/profiler/Profiler.cc 5d38dc2f7d66 > src/mem/ruby/slicc_interface/AbstractController.hh 5d38dc2f7d66 > src/mem/ruby/slicc_interface/AbstractController.cc 5d38dc2f7d66 > src/mem/ruby/slicc_interface/AbstractDMAController.hh PRE-CREATION > src/mem/ruby/slicc_interface/AbstractDMAController.cc PRE-CREATION > src/mem/ruby/slicc_interface/Controller.py 5d38dc2f7d66 > src/mem/ruby/slicc_interface/FirstLevelController.hh PRE-CREATION > src/mem/ruby/slicc_interface/FirstLevelController.cc PRE-CREATION > src/mem/ruby/slicc_interface/SConscript 5d38dc2f7d66 > src/mem/ruby/system/CacheRecorder.hh 5d38dc2f7d66 > src/mem/ruby/system/CacheRecorder.cc 5d38dc2f7d66 > src/mem/ruby/system/DMASequencer.hh 5d38dc2f7d66 > src/mem/ruby/system/DMASequencer.cc 5d38dc2f7d66 > src/mem/ruby/system/RubyPort.hh 5d38dc2f7d66 > src/mem/ruby/system/RubyPort.cc 5d38dc2f7d66 > src/mem/ruby/system/RubyPortProxy.hh 5d38dc2f7d66 > src/mem/ruby/system/RubySystem.cc 5d38dc2f7d66 > src/mem/ruby/system/RubySystem.py 5d38dc2f7d66 > src/mem/ruby/system/SConscript 5d38dc2f7d66 > src/mem/ruby/system/Sequencer.hh 5d38dc2f7d66 > src/mem/ruby/system/Sequencer.cc 5d38dc2f7d66 > src/mem/ruby/system/Sequencer.py 5d38dc2f7d66 > src/mem/slicc/symbols/StateMachine.py 5d38dc2f7d66 > tests/configs/pc-simple-timing-ruby.py 5d38dc2f7d66 > configs/example/fs.py 5d38dc2f7d66 > configs/ruby/MESI_Three_Level.py 5d38dc2f7d66 > configs/ruby/MESI_Two_Level.py 5d38dc2f7d66 > configs/ruby/MI_example.py 5d38dc2f7d66 > configs/ruby/MOESI_CMP_directory.py 5d38dc2f7d66 > configs/ruby/MOESI_CMP_token.py 5d38dc2f7d66 > configs/ruby/MOESI_hammer.py 5d38dc2f7d66 > configs/ruby/Network_test.py 5d38dc2f7d66 > configs/ruby/Ruby.py 5d38dc2f7d66 > src/mem/protocol/MESI_Three_Level-L0cache.sm 5d38dc2f7d66 > > Diff: http://reviews.gem5.org/r/3134/diff/ > > > Testing > ------- > > > Thanks, > > Nilay Vaish > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
