----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2901/#review6573 -----------------------------------------------------------
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. src/mem/ruby/network/Network.cc (line 68) <http://reviews.gem5.org/r/2901/#comment5673> Why does this need to change? If it's for consistency, why did you choose to declare local pointers everywhere and access them through params()? It seems superfluous. Are there forthcoming changes that rely on this convention? src/mem/ruby/network/simple/Throttle.hh (line 55) <http://reviews.gem5.org/r/2901/#comment5674> 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. src/mem/ruby/network/simple/Throttle.cc (line 250) <http://reviews.gem5.org/r/2901/#comment5672> Why not just use ruby_system as the pointer instead of creating a local pointer, rs? - Joel Hestness 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
