----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3235/#review7866 -----------------------------------------------------------
The implementation here looks good. Thanks for putting this together! Just a couple small requests below. src/cpu/kvm/base.cc (line 495) <http://reviews.gem5.org/r/3235/#comment6813> We should probably check with Andreas Sandberg on this one. It looks like ctrInsts is already a cummulative stat for the KVM CPU. This might be required by the KVM CPU for fast-forwarding correctness? This could cause changes in KVM correctness and/or regressions (if any are maintained). Hopefully, it is harmless to allow the stat to reset, though? src/cpu/minor/cpu.cc (line 335) <http://reviews.gem5.org/r/3235/#comment6814> Just a note that this is interesting... looks like this fixes an inconsistency: MinorCPU appears to collect inst stats cummulatively, while the other CPUs use resettable stats (another indicator that this patch will help with clarity!) src/sim/stat_control.cc (line 158) <http://reviews.gem5.org/r/3235/#comment6812> I'm hoping that we could use stat names that are a little more self-evident for these. Maybe instead of 'simulatorXxxx'/'simulator_xxxx' we could use something like 'cummulativeSimXxxx'/'cumm_sim_xxxx'? That way the names more clearly indicate that the stats are accumulated throughout the whole simulation time rather than just the current stats period. I do like that the descriptions say "not reset" also. - Joel Hestness On Jan. 6, 2016, 10:07 p.m., Lena Olson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3235/ > ----------------------------------------------------------- > > (Updated Jan. 6, 2016, 10:07 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11230:374d79447577 > --------------------------- > stats: organize and clarify resettable stats > > Some stats (e.g. sim_insts) were traditionally not reset, because they were > intended for simulator-level measurement. However, some other stats (e.g. > sim_ticks) WERE reset. In addition, some derived stats used both reset and > unreset stats in the calculation, resulting in nonsense values. This patch > creates two sets of overall stats: one set that respects reset, and is useful > for measuring the simulated program, and one set that does not reset, useful > for > studying the simulator itself. The non-resettable stats are prefixed with > "simulator_", as they behave different from other stats. > > > Diffs > ----- > > src/arch/null/cpu_dummy.hh 021524c21cbc > src/cpu/BaseCPU.py 021524c21cbc > src/cpu/base.hh 021524c21cbc > src/cpu/checker/cpu.hh 021524c21cbc > src/cpu/kvm/base.hh 021524c21cbc > src/cpu/kvm/base.cc 021524c21cbc > src/cpu/minor/cpu.cc 021524c21cbc > src/cpu/o3/cpu.hh 021524c21cbc > src/cpu/o3/cpu.cc 021524c21cbc > src/cpu/simple/base.hh 021524c21cbc > src/cpu/simple/base.cc 021524c21cbc > src/sim/stat_control.hh 021524c21cbc > src/sim/stat_control.cc 021524c21cbc > > Diff: http://reviews.gem5.org/r/3235/diff/ > > > Testing > ------- > > > Thanks, > > Lena Olson > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
