> On June 23, 2015, 3:38 p.m., Joel Hestness wrote: > > Overall, it seems like there should be a better way to do this using > > gem5-principled stats. However, it seems pretty non-trivial to use gem5 > > stats to handle, for instance, the VC load average stats without > > significant changes. > > > > Another option, which would clean things up significantly, would be to make > > start_cycle a private static member of RubySystem and provide a static > > accessor method. Just like warm-up and cool-down, stats reset is a > > simulation-wide operation and not system-instance-specific, so there only > > needs to be one start_cycle variable in the simulation. You're already > > resetting this variable like a static member, and you've already added the > > accessor function, so you'd just need to roll-out the RubySystem pointer > > changes to do it this way. This also removes the need to add parameters to > > the Throttle.
Just as a follow-up to this, I see now that the Throttles also poses significant complexity in handling cycle counts. Specifically, they're actually cycled by the Switch rather than the RubySystem, so their clocks should derive from the Switch. This looks like it would be quite tricky to fix, for a number of reasons. So, I'm fine with this patch after addressing minor things below. Thanks for the updates! > On June 23, 2015, 3:38 p.m., Joel Hestness wrote: > > src/mem/ruby/network/simple/Throttle.hh, line 55 > > <http://reviews.gem5.org/r/2901/diff/2/?file=46726#file46726line55> > > > > I don't understand why this is inconsistent with the GarnetNetwork > > changes: Here, you've added a per-instance pointer to Ruby to achieve the > > same functionality as with the GarnetNetwork changes, but in the > > GarnetNetwork, you're accessing the RubySystem from the params() in > > collateStats. > > > > I'd rather these be consistent. > > > > NOTE: If you were to follow my overall suggestion of making the > > start_time static in RubySystem, this change would not need to happen, and > > there would be no consistency issues between here and the GarnetNetwork. > > Brandon Potter wrote: > Throttle is not derived from a clocked memory object, but it does have a > clocked memory object that is passed into it. Is it reasonable to assume > that this clocked memory object will return the same cycle count as the > RubySystem object? > > If so, I think that it would be possible to avoid passing in the pointer > to RubySystem. If not, the RubySystem pointer needs to be passed in to give > the right cycle count with curCycle inside Throttle::collateStats(). Ok, I understand that you need to get the cycle count from somewhere and that you've chosen the RubySystem for that. What I'm confused about is the inconsistency of whether you add a member variable to point to the RubySystem or access it from the params. Common gem5 convention is to add a member variable any time that a param variable is used outside the constructor. Your changes to Throttle follow this convention. I think it would be more consistent to add member variables that point to the RubySystem from the GarnetNetwork objects. - Joel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2901/#review6573 ----------------------------------------------------------- On June 22, 2015, 4:36 p.m., Brandon Potter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2901/ > ----------------------------------------------------------- > > (Updated June 22, 2015, 4:36 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10876:8f73cf051cdd > --------------------------- > ruby: replace global cycle counter w/ cycle per ruby system > > > Diffs > ----- > > src/mem/ruby/network/Network.cc ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/network/garnet/fixed-pipeline/GarnetNetwork_d.cc > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/network/garnet/flexible-pipeline/GarnetNetwork.cc > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/network/simple/Switch.cc > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/network/simple/Throttle.hh > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/network/simple/Throttle.cc > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/system/System.hh ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/system/System.cc ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/common/Global.hh ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/common/Global.cc ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > > Diff: http://reviews.gem5.org/r/2901/diff/ > > > Testing > ------- > > > Thanks, > > Brandon Potter > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
