> On Nov. 12, 2015, 8:08 p.m., Joel Hestness wrote: > > src/mem/slicc/symbols/StateMachine.py, line 313 > > <http://reviews.gem5.org/r/3192/diff/2/?file=51487#file51487line313> > > > > 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). > > Brad Beckmann wrote: > 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?
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. - Joel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3192/#review7540 ----------------------------------------------------------- On Nov. 9, 2015, 9:16 p.m., Tony Gutierrez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3192/ > ----------------------------------------------------------- > > (Updated Nov. 9, 2015, 9:16 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11199:5d5bd1b0d332 > --------------------------- > ruby: split CPU and GPU latency stats > > > Diffs > ----- > > src/mem/ruby/profiler/Profiler.hh 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/profiler/Profiler.cc 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/slicc_interface/AbstractController.hh > 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/slicc_interface/AbstractController.cc > 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/slicc/symbols/StateMachine.py > 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > > Diff: http://reviews.gem5.org/r/3192/diff/ > > > Testing > ------- > > > Thanks, > > Tony Gutierrez > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
