> 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. > > Sooraj Puthoor wrote: > 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. > > Nilay Vaish wrote: > * 2550 and 2551 were posted in December 2014, GPU Coalescer has an AMD > copyright of 2013. I can point out discussions that happened last year and > may be this year as well where I pressed for not writing protocol specific > code in protocol independent files. > > * 2550 is completely unrelated to this discussion since the question > under discussion is whether or not to have statically defined machine types. > 2551 has only one accept and that is from Brad. > > * The reason to rewrite is not to have statically defined machine types. > My reasons against static definitions: > > -- no need for static definitions as of now. Creates an unncessary > requirement for all coherence protocols. > > -- the GPU coalescer code can be written without static machine types. > Add statistical variables to Coalescer class in RubySlicc_Types.sm, define > functions that modify the variables in the relevant .sm files and call those > functions where required. Take a look at how CacheMemory hit and miss counts > are maintained if you need to look at existing code. > > -- will at least double the size of NetDest, may be three times. There > are about 5 different machine types that might suffice for now and AMD is > proposing to add at least 5 more types (what ever I could infer from the > python config files) + the _wCC types. We would need at least 10 X 8bytes = > 80 bytes for just expressing the destination set of a message, which is > mostly going to be unused.
Nilay and Joel, we need to move past this problem. As you point out, the discussion on reviewboard has gone on for a year. We put a lot of effort trying to resolve the contention using your solution (see 2773). There is only so much effort you can expect from us, especially because this is *not* going to break your code. I understand your desire for the more elegant solution, but this is a research simulator. We need to move one. The bottomline is we need to provide flexibility to profile protocol specific information in protocol independent code. Here are our respones to your specific suggestions: - This requirement has existed for 10+ years in SLICC before your changes. The requirement is miminal. All one has to do is modify the enums. - Your suggestion to profile the latencies within the .sm files leads to a lot of duplicated code within each protocol. - If this is a concern, can we go back to defining the GenericMachineType? Would you be happy with that solution? - Brad ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3177/#review7444 ----------------------------------------------------------- On Dec. 23, 2015, 4:55 p.m., Tony Gutierrez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3177/ > ----------------------------------------------------------- > > (Updated Dec. 23, 2015, 4:55 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11277:a1ea750f25dc > --------------------------- > ruby: slicc: have a static MachineType > > This patch is imported from reviewboard patch 2551 by Nilay. > This patch moves from a dynamically defined MachineType to a statically > defined one. The need for this patch was felt since a dynamically defined > type prevents us from having types for which no machine definition may > exist. > > The following changes have been made: > i. each machine definition now uses a type from the MachineType enumeration > instead of any random identifier. This required changing the grammar and the > *.sm files. > ii. MachineType enumeration defined statically in RubySlicc_Exports.sm. > > > Diffs > ----- > > src/mem/protocol/MESI_Two_Level-L1cache.sm > d9a0136ab8cc4b3cf4821d064140b857e60db0dd > src/mem/protocol/MESI_Two_Level-L2cache.sm > d9a0136ab8cc4b3cf4821d064140b857e60db0dd > src/mem/protocol/MESI_Two_Level-dir.sm > d9a0136ab8cc4b3cf4821d064140b857e60db0dd > src/mem/protocol/MESI_Two_Level-dma.sm > d9a0136ab8cc4b3cf4821d064140b857e60db0dd > src/mem/protocol/RubySlicc_Exports.sm > d9a0136ab8cc4b3cf4821d064140b857e60db0dd > src/mem/slicc/symbols/SymbolTable.py > d9a0136ab8cc4b3cf4821d064140b857e60db0dd > src/mem/slicc/symbols/Type.py d9a0136ab8cc4b3cf4821d064140b857e60db0dd > src/mem/slicc/ast/DeclListAST.py d9a0136ab8cc4b3cf4821d064140b857e60db0dd > src/mem/slicc/ast/MachineAST.py d9a0136ab8cc4b3cf4821d064140b857e60db0dd > src/mem/slicc/parser.py d9a0136ab8cc4b3cf4821d064140b857e60db0dd > > 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
