> 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. > > Brandon Potter wrote: > 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.__ > > Nilay Vaish wrote: > Brandon, in April itself, I had said that I would like to see use of ruby > system ptr go down. > You were part of that debate, I expect you to be aware of my position. > My opinion on 3113 is > only stronger than what Jason and Joel posted. I refrained from > commenting since I do not want > put on fake politeness that people expect me to. > > > Conceptually, there's nothing wrong with doing it the way that it's > done in the patch. > > 3113 is (conceptually) saying that if I can possibly connect my computer > to the internet, then > my memory system needs to use the computer's ip address in all its > operations. We don't do > this in real designs, I don't see any need to do so in simulated designs. > On performance side, > you have already said it hurts performance, and I don't think we need > any more of that. > > > 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. > > I think you are assuming that new parameter will be as pervasive as block > size. Since the new parameter would be "new", so that's not true. > > There are three major things that need to be fixed to get multiple > systems working: block address, > statistical counters and machine types. Patches for all of them are on > the review board. While > I can get multiple systems working in may be about a day, even several > months are not > enough to get AMD to agree to write code in a fashion so that we do not > need to write it again. > > >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. > > This particular patch was originally written either late July or early > August. I could have > posted it then. Even without posting the patch, I knew what Brad's > response is going to be. > Since you posted 3113, I had to post this patch. > > The problem is get AMD to agree to anything. Read the recent discussions > related to wire buffer, > ruby random tester or Brad's proposed compromise on patches related to > SLICC or any other discussion > related to Ruby. I am not even unable to convince AMD to drop incorrect > code or not drop correct > code. I hardly think I can convince you or others at AMD to work with me > in improving the design > or performance of ruby. Ultimately, if you want to stick to 3113, I > don't think I'll be able to > stop you from doing so.
Nilay, please keep in mind this is a public forum and anyone in our community can read this. Potential future employers for all of us can reference this discussion. I would suggest maintaining politeness, even if it is fake. The problem we are struggling with is a lack of communication and coordination. You mentioned that you developed 3121 in late-July/early-August. I noticed that Brandon posted 2902 and 2903 on 6/18. You’ve contributed to many recent enhancements to the infrastructure of Ruby and we appreciate all the improvements you have made. However from AMD’s perspective, this patch seems like a competitive reaction to Brandon’s code. This is difficult code to write once. It seems rather unnecessary to have had separate people write it twice. Also you say that your "opinion on 3113 is only stronger than what Jason and Joel posted", implying that Jason & Joel didn’t like Brandon's patch. We went back and reread their comments though, and while they had some pretty substantial concerns about the implementation details (and constructive ideas about how to improve things), they both seemed OK with the basic premise of giving every object a pointer to RubySystem. You seem to be the only one against that aspect of Brandon's patch. This is simulation and there is tremendous value for passing global information across the modules. If we restrict the visibility across modules, then we might as well use a higher abstraction evaluation tool. Overall, multi-node Ruby is a valuable and necessary feature, and Brandon's patch demonstrates a full solution to that problem. Furhtermore, Brandon's patch has been thoroughly tested and used by multiple developers. Meanwhile your patches do not demonstrate a complete solution, so let's go with what we know works. - Brad ----------------------------------------------------------- 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
