Please provide a description for this patch.

src/mem/protocol/RubySlicc_Exports.sm (Diff revision 1)

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?

Show all issues

1. Sooraj Puthoor Nov. 12, 2015, 9:37 a.m. (Nov. 12, 2015, 9:37 a.m.)

   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.

2. Brad Beckmann Nov. 12, 2015, 10:14 a.m. (Nov. 12, 2015, 10:14 a.m.)

   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.

3. Nilay Vaish Nov. 12, 2015, 10:34 a.m. (Nov. 12, 2015, 10:34 a.m.)

   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.

4. Joel Hestness Nov. 12, 2015, 10:44 a.m. (Nov. 12, 2015, 10:44 a.m.)

   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.

5. Brad Beckmann Nov. 12, 2015, 10:55 a.m. (Nov. 12, 2015, 10:55 a.m.)

   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?

6. Nilay Vaish Nov. 12, 2015, 11:13 a.m. (Nov. 12, 2015, 11:13 a.m.)

   
   
   
   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?

7. Joel Hestness Nov. 12, 2015, 11:18 a.m. (Nov. 12, 2015, 11:18 a.m.)

   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.

8. Sooraj Puthoor Nov. 20, 2015, 12:50 p.m. (Nov. 20, 2015, 12:50 p.m.)

   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.

9. Nilay Vaish Nov. 21, 2015, 9:10 p.m. (Nov. 21, 2015, 9:10 p.m.)

   
   
   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.

10. Brad Beckmann Dec. 23, 2015, 10:25 a.m. (Dec. 23, 2015, 10:25 a.m.)

    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?