Ship It!

src/mem/ruby/profiler/Profiler.hh (Diff revision 1)

I don't see where this guy is used. Can you please verify that this is used in another patch, so we don't add dead code?

Show all issues

1. John Alsop Nov. 9, 2015, 2:51 p.m. (Nov. 9, 2015, 2:51 p.m.)

   The histograms added will be populated when using GPUCoalescer objects added in patch 3189. Is that what you're asking?

2. Joel Hestness Nov. 10, 2015, 2:39 p.m. (Nov. 10, 2015, 2:39 p.m.)

   Apologies for the confusion. In the first diff, it appeared that either the old histogram or one of the new ones was unnecessary. That is clarified in the new diff.

3. John Alsop Nov. 12, 2015, 11:55 a.m. (Nov. 12, 2015, 11:55 a.m.)

   No worries- yeah, the new changes were needed to make the stats work with GPUCoalescer. Did you mean to give this a ship-it or are you still reviewing the newer diff?

I can give a "Ship it" here if we decide a reasonable way to tackle the issue below.

src/mem/slicc/symbols/StateMachine.py (Diff revision 2)

Not sure if this should be fixed before commit, but I feel it should be fixed at some point:
It seems like we should be using better inheritance here. For the interface with a cache state machine, is there a difference between a Sequencer and a Coalescer? If not, these getCPUSequencer and getGPUCoalescer functions should be merged to return a type from which both Sequencer and GPUCoalescer descend (e.g. in review request 3189, GPUCoalescer copies much of the Sequencer functionality, so it seems like GPUCoalescer should descend from Sequencer and just overload functionality). Elsewhere in this patch where you need to decide whether to accumulate sequencer or coalescer stats, we could use an accessor function in the common ancestor type that indicates whether the attached RubyPort/Sequencer is a "sequencer" or "coalescer". The interfaces for these types appear nearly identical, and this would keep the modifications to SLICC to a minimum (e.g. these changes would not be necessary).

Show all issues

1. Brad Beckmann Nov. 18, 2015, 11:43 a.m. (Nov. 18, 2015, 11:43 a.m.)

   There is a difference between the GPUCoalescer and Sequencer APIs.  Though they have similar named functions, they require different arguments and behave very differently.  Also note that it GPUCoalescer and/or Sequencer pointers are passed into the machines, not RubyPort pointers.  I think it would be very hard to change that.
   The GPUCoalescer behaves very differently than the Sequencer.  If you actually diff the cc files, you will notice that well over 90% of the lines are different and that no two functions are the same.  There is some Alpha LLSC cruft in the GPUCoalescer that doesn't make much sense to be there.  Perhaps that is why you think there is a lot of copying between the two files?  Would you feel better about this patch if we removed the LLSC cruft from the GPUCoalescer in 3189?

2. Joel Hestness Nov. 20, 2015, 12:10 p.m. (Nov. 20, 2015, 12:10 p.m.)

   Apologies for the push here, but I don't feel you're representing these differences accurately. It looks trivial to align and unify these C++ classes. Let's break it down:
   1) Their current common APIs: Both Sequencers and GPUCoalescers are RubyPorts, so they implement that common API. Nothing to do here.
   2) Beyond that, Sequencers implement read and write callbacks from SLICC machines, and GPUCoalescers implement atomic callbacks. Sure, the parameters for GPUCoaleser callbacks are different from Sequencer callbacks, but it's because no effort has been made to unify them; Specifically, all of the read AND write callbacks in GPUCoalescer just call the callback with the most generic parameter list. The same effect can be achieved using default parameters, unifying these callbacks with the Sequencer callbacks. A Sequencer may implement the atomicCallback in the future (e.g. we could use it with the current organization of gem5-gpu), so it can be added to the common API.
   3) Finally, for now, the recordCP*CallBacks can be left as members of GPUCoaleser, though they are pretty hacky: They record stats in coalescers when data is returned through sequencers. However, if there was better Sequencer/GPUCoalescer abstraction, you would only need to connect a sequencer OR a coalescer to those state machines (rather than both), AND you could eliminate the strange use_seq_not_coal variable.
   Overall, I feel strongly that Sequencers and GPUCoalescers need to be interchangeable at the interface to cache controllers, because it is likely that users will want flexibility to use either (your GPU protocols already suggest that you're hacking around unifying their interfaces - e.g. MachineType:TCP). It would be nice if some more thought could go into how and when to make this happen.

3. Brad Beckmann Dec. 3, 2015, 4:54 p.m. (Dec. 3, 2015, 4:54 p.m.)

   1) Yes, they implement the same APIs, but their implementations are very different
   
   2) Unifying the parameters is a lot of work.  Even if we unified the parameters, the callback functionality is different.  For instance Seq->hitcallback only process one pkt, where Coal->hitcallback processes several packet responses using a two phase approach (hitcallback calls completeHitCallback). 
   
   3) I'm not against what you suggest, but don't let that hang up this patch.  There is a lot of subtle issues trying to accomplish interchangeablity.  As I recall, we tried to do that but we determined it would not be easy and it wasn't worth the effort.
   Can you please just let us check in this patch?

4. Joel Hestness Dec. 4, 2015, 8:43 a.m. (Dec. 4, 2015, 8:43 a.m.)

   Sure. I was merely suggesting that we should have a plan for how this gets addressed. It would be my preference that AMD plan to merge the Sequencer and GPUCoalescer at a later date. Is that agreeable?

5. Brad Beckmann Dec. 4, 2015, 10:39 a.m. (Dec. 4, 2015, 10:39 a.m.)

   I agree at a later date we will revamp the entire Sequencer/Coalescer design.  In particular, as I described to you a couple months ago, I want to decouple the storage between pending requests and issued requests.  I think that will resolve a lot of confusion.

6. Joel Hestness Dec. 4, 2015, 10:44 a.m. (Dec. 4, 2015, 10:44 a.m.)

   Ok. Thank you!

Ship It!