> On Nov. 2, 2015, 8:56 p.m., Joel Hestness wrote: > > Please provide a description for this patch. > > Sooraj Puthoor wrote: > This patch is imported from reviewboard and the original patch is > http://reviews.gem5.org/r/2551/.So, would it help if we use the same > description used in that patch for this patch also? > > Joel Hestness wrote: > Sure...? All patches need descriptions (see: > http://gem5.org/Submitting_Contributions ). Please update this patch's > description.
Thanks. Will update the patch description in the next revision of this patch. > On Nov. 2, 2015, 8:56 p.m., Joel Hestness wrote: > > src/mem/protocol/RubySlicc_Exports.sm, line 189 > > <http://reviews.gem5.org/r/3177/diff/1/?file=50930#file50930line189> > > > > I don't understand the reason for distinguising _wCC types. Should > > L1Cache/L2Cache types not provide cache coherence? > > > > More generally, if we're going to define these statically, we should > > make sure that they are descriptive and provide a fairly complete set of > > machine type names for existing protocols. Can you please update these and > > provide clearer comments as appropriate? > > Sooraj Puthoor wrote: > I do not know the history behind this change. This is originally a patch > from Nilay (http://reviews.gem5.org/r/2551) and I think Nilay will be the > best person to answer this question. > > Brad Beckmann wrote: > The wCC names have existed for 10+ years. The CC actually stands for > Cache-to-Cache and was used to identify which responses required > cache-to-cache transfers. Since a remote L1 or L2 cache could respond to a > request, there are separate L1Cache_wCC and L2Cache_wCC. > > The existing comments and type descriptions are already fairly long. Do > we really need to expand them? I think you are being pretty unreasonable > here. > > Nilay Vaish wrote: > In my opinion neither 3176 nor 3177 are required. I have seen the patch > on the gpu model and how it uses MachineType in GPUCoalescer. As and when > AMD breaks that patch into smaller pieces, I'll re-write some portions so > that we do not need any statically defined machine types. > > Joel Hestness wrote: > If we can pull in the GPU without static MachineTypes, excellent! That's > what I'd prefer, so we wouldn't need this patch. > > If we're going to reintroduce static MachineTypes with this patch, the > intended meaning of "_wCC" is currently different than implied in the types' > descriptions. This is very confusing. It is very reasonable to ask that these > have more precise/accurate descriptions. > > Brad Beckmann wrote: > Nilay, we will split our gpu baseline patch and are going to use the > standard review-checkin process. Nilay, if you want to propose further > updates to the model, you are welcome to do that after we check them in. We > will be happy to review them and go throught the same standard review-checkin > process. I do not want you rewriting our code before we check it in. > > Joel, the descriptions are precise and accurate. Why are you being such > a pain here? > > Nilay Vaish wrote: > > Nilay, we will split our gpu baseline patch and are going to use the > standard review-checkin process. Nilay, if you want to propose further > updates to the model, you are welcome to do that after we check them in. We > will be happy to review them and go throught the same standard review-checkin > process. I do not want you rewriting our code before we check it in. > > Can you explain to me the point of the review process if you are against > re-writing patches? Why not commit them straight away? > > Joel Hestness wrote: > I'm sorry if this is coming across as me being a pain. That's not what > I'm intending. > > I feel that there is clear miscommunication about what "_wCC" refers to > in the current code, because the descriptions on the "_wCC" types are > ambiguous/inaccurate: There are two pairs of types like this (L1Cache vs. > L1Cache_wCC, and L2Cache vs. L2Cache_wCC). The current type descriptions on > these pairs appear to imply that the "_wCC" types mean "with Cache > Coherence", because this is the only thing different about their > descriptions. However, "_wCC" certainly does not mean "with cache coherence", > because basically all existing state machines work together to provide cache > coherence. The current descriptions confuse me. > > I'm not sure I understand what you mean by "CC actually stands for > Cache-to-Cache and was used to identify which responses required > cache-to-cache transfers", but it's clear that what you're trying to describe > here more accurate than "with cache coherence" in the current type > descriptions. Do you mean that the "_wCC" types are used to describe > cache-to-cache messages between state machines of the types without "_wCC" > (i.e. you can have L1Cache_wCC messages between machines of type L1Cache)? If > so, this seems like misuse of MachineTypes, because these are not types of > machines, but rather types of messages between machines. If not, there needs > to be clearer explanation of why we have these types. Joel, Thanks for the clarification. And thanks for pointing out the deficiency in the existing description of _wCC. Will update the description in the next revision of this patch. Please revisit the patch after it is updated and confirm if the updated description is clear. Nilay, Thanks for volunterring to re-write this patch. GPU Coalescer was written assuming that patches 2550 and 2551 were going to be checked in since both those patches got shipits wihtout any objection from reviewers. Actually, the patches 3176 and 3177 are imports of patches 2550 and 2551. So, it would be great to let these patches checked in without any rewrite. Probably you can rewrite it later after we checked in our changes. Obviously, I am assuming that you are proposing a rewrite solely because our GPU patches are dependent on "3176 nor 3177". Again, I could be wrong in understanding your proposal for a rewrite but I could not find any more detail about this rewrite proposal in the comments so far. - Sooraj ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3177/#review7444 ----------------------------------------------------------- On Oct. 30, 2015, 9:49 p.m., Tony Gutierrez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3177/ > ----------------------------------------------------------- > > (Updated Oct. 30, 2015, 9:49 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11183:4565e2649f8f > --------------------------- > ruby: imported from reviewboard patch 2551 > > filename temp_nilay_machtype.patch > > > Diffs > ----- > > src/mem/slicc/symbols/Type.py 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/slicc/symbols/SymbolTable.py > 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/slicc/parser.py 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/slicc/ast/MachineAST.py 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/slicc/ast/DeclListAST.py 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/protocol/MESI_Two_Level-L1cache.sm > 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/protocol/MESI_Two_Level-L2cache.sm > 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/protocol/MESI_Two_Level-dir.sm > 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/protocol/MESI_Two_Level-dma.sm > 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/protocol/RubySlicc_Exports.sm > 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > > Diff: http://reviews.gem5.org/r/3177/diff/ > > > Testing > ------- > > > Thanks, > > Tony Gutierrez > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
