----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3235/#review7942 -----------------------------------------------------------
I'm glad you've taken the time to figure out what all these stats actually do. I like the direction, but a couple of comments. First, can you add more comments to the code. Similar to what's in o3/cpu.hh. It would be good if this was at least in all of the .hh files, maybe in the .cc files too. Related to my next comment, but if the names aren't obvious, they should at least be very well commented. Second, these names make no sense to me. Why are "sim*" reset and "simulator*" not reset? Isn't "sim" short for "simulator" or "simulation"? My suggestion: For statitics that are reset every time, have no prefix at all so there would be `ticks`, `instructions`, etc. We already know the came from a simulator/simulation. For the statistics that don't reset you could call them "total*" (i.e., `totalTicks`, `totalInsts`). And I think `finalTicks` is fine as it is with its extra comments. - Jason Lowe-Power 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 [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
