> 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. > > Brad Beckmann wrote: > 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. > > Nilay Vaish wrote: > Why would a DMA controller issue PIO requests?
One more thing, there is a line in fs.py that attaches piobus to a ruby controller. It might be that that is what you are missing. - Nilay ----------------------------------------------------------- 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
