> On Sept. 17, 2015, 5:43 p.m., Brad Beckmann wrote: > > Do *not* check in this patch. I know I've already saidy this, but I will > > repeat, do not make further changes to the sm files until we check in our > > GPU patches. > > > > Even after we check in our GPU patches, this patch must be split up before > > it should be considered for review. This is far too extensive and the > > changes in this patch go well beyond the patch description. First off, > > there are many, many unrelated style changes in this patch. Second, you > > cannot remove the Subblock object. The Subblock object is an important > > data structure for all testers. Beyond the RubyTester, we have internal > > testers that rely on the Subblock object. > > Nilay Vaish wrote: > * I don't see the point of having SubBlock once the DataBlock class also > maintains a size of its own. > > * I don't think it would be possible to split the patch. The patch > started of with making members in Ruby non-static. Even if we were to leave > them as is, changing makeLinkAddress(address) to makeLineAddress(address, > block size bits) would introduce similar amount of changes. i think we will > have to go with the patch as is. That said, I am willing to consider ways in > which we can break the patch. > > Brad Beckmann wrote: > - We don't want to update our code. What is the harm of keeping the > SubBlock around? > - I'd like to get Brandon to comment on the big-picture philosophical > changes in this patch versus what he's been working on. Depending on what he > thinks, I may give this a detailed review and point out where this patch can > be split.
Nilay, functionally this achieves the same thing as http://reviews.gem5.org/r/3113/. I know that my patch needs a fair amount of work, but would you mind posting a review for that patch instead of posting patches like this one (at least for now). Have you managed to solve the other problems and run a multi-system Ruby simulation? If so, it's conceivable that we could use your entire patchset instead. If not, I'd prefer to use mine. Conceptually, there's nothing wrong with doing it the way that it's done in the patch. However, if an object requires multiple variables then it does mean that you'll need to pass multiple parameters through instead of just a single RubySystem pointer. One of the benefits of the RubySystem pointer is that many of the objects will already have it. It won't take much effort to add a new field to RubySystem if a new field is needed in the future. If we pass parameters (as is done in this patch), then there will be more patches like this one in the future. __My only concern is enabling the functionality now.__ Once that's done, the RubySystem class members which should be per-object can be moved into their proper place if there's consensus on it. I will not argue against the change at that time as long as it does not break any functionality. If you want to clean-up the code or try to improve the performance then I feel that's commendable; I don't have a problem with it. If this were to go into the mainline as a changeset, it would take a considerable amount of time to refactor 3113 (which does enable multi-instance simulation). __TL;DR: I think that it would be much better if we worked together on the solution rather than have a competition to see if we can solve the same problem in two completely different ways.__ - Brandon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3121/#review7207 ----------------------------------------------------------- On Sept. 17, 2015, 1:18 a.m., Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3121/ > ----------------------------------------------------------- > > (Updated Sept. 17, 2015, 1:18 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11124:3030ff525efb > --------------------------- > ruby: block size in RubySystem to be object specific > > This patch started with changing the variable for block size bits and bytes in > RubySystem from static to object specific. Most of the other changes made > were > required to support the original changes. > > > Diffs > ----- > > src/mem/ruby/network/garnet/fixed-pipeline/VirtualChannel_d.cc 5a2e1b1b5c43 > src/mem/ruby/network/garnet/fixed-pipeline/flitBuffer_d.hh 5a2e1b1b5c43 > src/mem/ruby/network/garnet/fixed-pipeline/flitBuffer_d.cc 5a2e1b1b5c43 > src/mem/ruby/network/garnet/fixed-pipeline/flit_d.hh 5a2e1b1b5c43 > src/mem/ruby/network/garnet/fixed-pipeline/flit_d.cc 5a2e1b1b5c43 > src/mem/ruby/network/garnet/flexible-pipeline/GarnetNetwork.hh 5a2e1b1b5c43 > src/mem/ruby/network/garnet/flexible-pipeline/GarnetNetwork.cc 5a2e1b1b5c43 > src/mem/ruby/network/garnet/flexible-pipeline/NetworkInterface.hh > 5a2e1b1b5c43 > src/mem/ruby/network/garnet/flexible-pipeline/NetworkInterface.cc > 5a2e1b1b5c43 > src/mem/ruby/network/garnet/flexible-pipeline/NetworkLink.hh 5a2e1b1b5c43 > src/mem/ruby/network/garnet/flexible-pipeline/NetworkLink.cc 5a2e1b1b5c43 > src/mem/ruby/network/garnet/flexible-pipeline/Router.hh 5a2e1b1b5c43 > src/mem/ruby/network/garnet/flexible-pipeline/Router.cc 5a2e1b1b5c43 > src/mem/ruby/network/garnet/flexible-pipeline/flit.hh 5a2e1b1b5c43 > src/mem/ruby/network/garnet/flexible-pipeline/flit.cc 5a2e1b1b5c43 > src/mem/ruby/network/garnet/flexible-pipeline/flitBuffer.hh 5a2e1b1b5c43 > src/mem/ruby/network/garnet/flexible-pipeline/flitBuffer.cc 5a2e1b1b5c43 > src/mem/ruby/network/simple/SimpleNetwork.hh 5a2e1b1b5c43 > src/mem/ruby/network/simple/SimpleNetwork.cc 5a2e1b1b5c43 > src/mem/ruby/network/simple/Switch.hh 5a2e1b1b5c43 > src/mem/ruby/network/simple/Switch.cc 5a2e1b1b5c43 > src/mem/ruby/profiler/AddressProfiler.cc 5a2e1b1b5c43 > src/mem/ruby/slicc_interface/AbstractController.hh 5a2e1b1b5c43 > src/mem/ruby/slicc_interface/AbstractController.cc 5a2e1b1b5c43 > src/mem/ruby/slicc_interface/Controller.py 5a2e1b1b5c43 > src/mem/ruby/slicc_interface/Message.hh 5a2e1b1b5c43 > src/mem/ruby/slicc_interface/RubyRequest.hh 5a2e1b1b5c43 > src/mem/ruby/slicc_interface/RubyRequest.cc 5a2e1b1b5c43 > src/mem/ruby/slicc_interface/RubySlicc_Util.hh 5a2e1b1b5c43 > src/mem/ruby/structures/CacheMemory.hh 5a2e1b1b5c43 > src/mem/ruby/structures/CacheMemory.cc 5a2e1b1b5c43 > src/mem/ruby/structures/DirectoryMemory.hh 5a2e1b1b5c43 > src/mem/ruby/structures/DirectoryMemory.cc 5a2e1b1b5c43 > src/mem/ruby/structures/DirectoryMemory.py 5a2e1b1b5c43 > src/mem/ruby/structures/PerfectCacheMemory.hh 5a2e1b1b5c43 > src/mem/ruby/structures/PersistentTable.cc 5a2e1b1b5c43 > src/mem/ruby/structures/Prefetcher.hh 5a2e1b1b5c43 > src/mem/ruby/structures/Prefetcher.cc 5a2e1b1b5c43 > src/mem/ruby/structures/RubyCache.py 5a2e1b1b5c43 > src/mem/ruby/structures/RubyPrefetcher.py 5a2e1b1b5c43 > src/mem/ruby/structures/TBETable.hh 5a2e1b1b5c43 > src/mem/ruby/structures/TimerTable.cc 5a2e1b1b5c43 > src/mem/ruby/system/CacheRecorder.hh 5a2e1b1b5c43 > src/mem/ruby/system/CacheRecorder.cc 5a2e1b1b5c43 > src/mem/ruby/system/DMASequencer.hh 5a2e1b1b5c43 > src/mem/ruby/system/DMASequencer.cc 5a2e1b1b5c43 > src/mem/ruby/system/RubyPort.cc 5a2e1b1b5c43 > src/mem/ruby/system/RubySystem.hh 5a2e1b1b5c43 > src/mem/ruby/system/RubySystem.cc 5a2e1b1b5c43 > src/mem/ruby/system/Sequencer.hh 5a2e1b1b5c43 > src/mem/ruby/system/Sequencer.cc 5a2e1b1b5c43 > src/mem/ruby/system/Sequencer.py 5a2e1b1b5c43 > src/mem/slicc/ast/ObjDeclAST.py 5a2e1b1b5c43 > src/mem/slicc/symbols/StateMachine.py 5a2e1b1b5c43 > src/mem/ruby/network/garnet/fixed-pipeline/Router_d.cc 5a2e1b1b5c43 > src/mem/ruby/network/garnet/fixed-pipeline/Switch_d.hh 5a2e1b1b5c43 > src/mem/ruby/network/garnet/fixed-pipeline/Switch_d.cc 5a2e1b1b5c43 > src/mem/ruby/network/garnet/fixed-pipeline/VirtualChannel_d.hh 5a2e1b1b5c43 > src/mem/ruby/network/garnet/fixed-pipeline/NetworkInterface_d.hh > 5a2e1b1b5c43 > src/mem/ruby/network/garnet/fixed-pipeline/NetworkInterface_d.cc > 5a2e1b1b5c43 > src/mem/ruby/network/garnet/fixed-pipeline/NetworkLink_d.hh 5a2e1b1b5c43 > src/mem/ruby/network/garnet/fixed-pipeline/NetworkLink_d.cc 5a2e1b1b5c43 > src/mem/ruby/network/garnet/fixed-pipeline/OutputUnit_d.hh 5a2e1b1b5c43 > src/mem/ruby/network/garnet/fixed-pipeline/OutputUnit_d.cc 5a2e1b1b5c43 > src/mem/ruby/network/garnet/fixed-pipeline/Router_d.hh 5a2e1b1b5c43 > src/mem/ruby/network/garnet/fixed-pipeline/InputUnit_d.cc 5a2e1b1b5c43 > src/cpu/testers/directedtest/RubyDirectedTester.hh 5a2e1b1b5c43 > src/cpu/testers/rubytest/Check.hh 5a2e1b1b5c43 > src/cpu/testers/rubytest/Check.cc 5a2e1b1b5c43 > src/cpu/testers/rubytest/CheckTable.hh 5a2e1b1b5c43 > src/cpu/testers/rubytest/CheckTable.cc 5a2e1b1b5c43 > src/cpu/testers/rubytest/RubyTester.hh 5a2e1b1b5c43 > src/cpu/testers/rubytest/RubyTester.cc 5a2e1b1b5c43 > src/cpu/testers/rubytest/RubyTester.py 5a2e1b1b5c43 > src/mem/protocol/MESI_Three_Level-L0cache.sm 5a2e1b1b5c43 > src/mem/protocol/MESI_Three_Level-L1cache.sm 5a2e1b1b5c43 > src/mem/protocol/MESI_Three_Level-msg.sm 5a2e1b1b5c43 > src/mem/protocol/MESI_Two_Level-L1cache.sm 5a2e1b1b5c43 > src/mem/protocol/MESI_Two_Level-L2cache.sm 5a2e1b1b5c43 > src/mem/protocol/MESI_Two_Level-dir.sm 5a2e1b1b5c43 > src/mem/protocol/MESI_Two_Level-dma.sm 5a2e1b1b5c43 > src/mem/protocol/MESI_Two_Level-msg.sm 5a2e1b1b5c43 > src/mem/protocol/MI_example-cache.sm 5a2e1b1b5c43 > src/mem/protocol/MI_example-dir.sm 5a2e1b1b5c43 > src/mem/protocol/MI_example-msg.sm 5a2e1b1b5c43 > src/mem/protocol/MOESI_CMP_directory-L1cache.sm 5a2e1b1b5c43 > src/mem/protocol/MOESI_CMP_directory-L2cache.sm 5a2e1b1b5c43 > src/mem/protocol/MOESI_CMP_directory-dir.sm 5a2e1b1b5c43 > src/mem/protocol/MOESI_CMP_directory-dma.sm 5a2e1b1b5c43 > src/mem/protocol/MOESI_CMP_directory-msg.sm 5a2e1b1b5c43 > src/mem/protocol/MOESI_CMP_token-L1cache.sm 5a2e1b1b5c43 > src/mem/protocol/MOESI_CMP_token-L2cache.sm 5a2e1b1b5c43 > src/mem/protocol/MOESI_CMP_token-dir.sm 5a2e1b1b5c43 > src/mem/protocol/MOESI_CMP_token-msg.sm 5a2e1b1b5c43 > src/mem/protocol/MOESI_hammer-cache.sm 5a2e1b1b5c43 > src/mem/protocol/MOESI_hammer-dir.sm 5a2e1b1b5c43 > src/mem/protocol/MOESI_hammer-msg.sm 5a2e1b1b5c43 > src/mem/protocol/Network_test-cache.sm 5a2e1b1b5c43 > src/mem/protocol/Network_test-msg.sm 5a2e1b1b5c43 > src/mem/protocol/RubySlicc_Defines.sm 5a2e1b1b5c43 > src/mem/protocol/RubySlicc_Exports.sm 5a2e1b1b5c43 > src/mem/protocol/RubySlicc_MemControl.sm 5a2e1b1b5c43 > src/mem/protocol/RubySlicc_Util.sm 5a2e1b1b5c43 > src/mem/ruby/common/Address.hh 5a2e1b1b5c43 > src/mem/ruby/common/Address.cc 5a2e1b1b5c43 > src/mem/ruby/common/DataBlock.hh 5a2e1b1b5c43 > src/mem/ruby/common/DataBlock.cc 5a2e1b1b5c43 > src/mem/ruby/common/SConscript 5a2e1b1b5c43 > src/mem/ruby/common/SubBlock.hh 5a2e1b1b5c43 > src/mem/ruby/common/SubBlock.cc 5a2e1b1b5c43 > src/mem/ruby/filters/AbstractBloomFilter.hh 5a2e1b1b5c43 > src/mem/ruby/filters/AbstractBloomFilter.cc PRE-CREATION > src/mem/ruby/filters/BlockBloomFilter.hh 5a2e1b1b5c43 > src/mem/ruby/filters/BlockBloomFilter.cc 5a2e1b1b5c43 > src/mem/ruby/filters/BulkBloomFilter.hh 5a2e1b1b5c43 > src/mem/ruby/filters/BulkBloomFilter.cc 5a2e1b1b5c43 > src/mem/ruby/filters/H3BloomFilter.hh 5a2e1b1b5c43 > src/mem/ruby/filters/H3BloomFilter.cc 5a2e1b1b5c43 > src/mem/ruby/filters/LSB_CountingBloomFilter.hh 5a2e1b1b5c43 > src/mem/ruby/filters/LSB_CountingBloomFilter.cc 5a2e1b1b5c43 > src/mem/ruby/filters/MultiBitSelBloomFilter.hh 5a2e1b1b5c43 > src/mem/ruby/filters/MultiBitSelBloomFilter.cc 5a2e1b1b5c43 > src/mem/ruby/filters/MultiGrainBloomFilter.hh 5a2e1b1b5c43 > src/mem/ruby/filters/MultiGrainBloomFilter.cc 5a2e1b1b5c43 > src/mem/ruby/filters/NonCountingBloomFilter.hh 5a2e1b1b5c43 > src/mem/ruby/filters/NonCountingBloomFilter.cc 5a2e1b1b5c43 > src/mem/ruby/filters/SConscript 5a2e1b1b5c43 > src/mem/ruby/network/MessageBuffer.hh 5a2e1b1b5c43 > src/mem/ruby/network/MessageBuffer.cc 5a2e1b1b5c43 > src/mem/ruby/network/Network.hh 5a2e1b1b5c43 > src/mem/ruby/network/Network.cc 5a2e1b1b5c43 > src/mem/ruby/network/Network.py 5a2e1b1b5c43 > src/mem/ruby/network/garnet/fixed-pipeline/GarnetNetwork_d.hh 5a2e1b1b5c43 > src/mem/ruby/network/garnet/fixed-pipeline/GarnetNetwork_d.cc 5a2e1b1b5c43 > src/mem/ruby/network/garnet/fixed-pipeline/InputUnit_d.hh 5a2e1b1b5c43 > > Diff: http://reviews.gem5.org/r/3121/diff/ > > > Testing > ------- > > All the protocols compile and pass hello world. > > > Thanks, > > Nilay Vaish > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
