> On June 4, 2014, 8:15 p.m., Steve Reinhardt wrote: > > src/cpu/minor/Expr.py, line 48 > > <http://reviews.gem5.org/r/2279/diff/1/?file=39827#file39827line48> > > > > This is pretty interesting... it would be nice to generalize this > > capability and not make it Minor-specific > > Andrew Bardsley wrote: > Yeah. MinorExpr could easily be hoisted upwards and lose its Minor > prefix (It doesn't have any dependencies on Minor). > > Maybe TimingExpr or something like that (to disambiguate and suggest its > function a bit more) would be a good name?
In r2: I've moved Minor::Expr up to be in src/cpu as TimingExpr > On June 4, 2014, 8:15 p.m., Steve Reinhardt wrote: > > src/cpu/minor/ticked.hh, line 60 > > <http://reviews.gem5.org/r/2279/diff/1/?file=39861#file39861line60> > > > > This seems like it could be useful outside of Minor... perhaps we > > should put it in src/sim, or even just integrate this functionality with > > ClockedObject > > Andrew Bardsley wrote: > Yes, the 'evaluate' member function on Ticked could be hoisted up to > ClockedObject. The cycle accounting in TickedModule could potentially also > go that way. I didn't make that change as I didn't want to try and have the > interface blessed/promoted by trying to affect a core class (and set > precedent and pass the tighter review that that demands). > > You can see, in Ticked, my preference for building 'interface'-like > classes and using multiple inheritance for interface-link classes. > In this case, Ticked isn't used (any more, it was previously) in contexts > where the 'virtual' qualification on its member functions is used anymore, so > arguably it isn't really functioning as a base class anymore. > > I would suggest dropping Ticked and just folding its member functions > into Stage and TickedModule. (I don't believe 'Ticked */&' is ever used) In r2: In the end I didn't remove Ticked. It felt wrong to remove such an obviously 'base'/interface definition. > On June 4, 2014, 8:15 p.m., Steve Reinhardt wrote: > > src/cpu/minor/ticked.hh, line 67 > > <http://reviews.gem5.org/r/2279/diff/1/?file=39861#file39861line67> > > > > We also have various objects that have dump() or print() methods... it > > would be nice to standardize this > > Andrew Bardsley wrote: > Actually, report is specifically used here for MinorTrace. Its name > possibly should reflect just that. Maybe Ticked should have been be > MinorTicked? ...::report should almost certainly really be ...::minorTrace > to reflect this specialised use. > > I think there are three interesting opportunities here: > > 1) Standardise a name/maybe some flags for micro-architectural dumping > 2) Standardise a format for human-readable dump > 3) Standardise a format for machine-readable dump > > MinorTrace is horribly verbose but a it's a crack at doing point 3. In r2: I've renamed 'report' to 'minorTrace' to reflect its very specific role in Minor. > On June 4, 2014, 8:15 p.m., Steve Reinhardt wrote: > > src/cpu/minor/ticked.hh, line 74 > > <http://reviews.gem5.org/r/2279/diff/1/?file=39861#file39861line74> > > > > integrating with ClockedObject would eliminate the multiple > > inheritance... not that MI is totally evil, but it's always better to avoid > > it when possible > > Andrew Bardsley wrote: > I think I've answered that one above. In r2: see above > On June 4, 2014, 8:15 p.m., Steve Reinhardt wrote: > > src/cpu/minor/trace.hh, line 63 > > <http://reviews.gem5.org/r/2279/diff/1/?file=39862#file39862line63> > > > > This would be very useful outside of Minor... should go in > > src/base/trace.hh, IMO > > Andrew Bardsley wrote: > Yes, it would and it could be used to clean up some of annoying > ambiguities around DPRINTF use and maybe even replace the DPRINTF macros with > functions. > > Using Named to replace _name/name in SimObject would sort of open up the > interface-like MI question. (Named isn't actually an interface as it has a > data member). > > I suggest moving Named into src/base/trace.hh so it can be optionally > used, but not go as far as trying to strongly suggest its usage by making > SimObject adopt it. Thoughts? In r2: I've moved Named into src/base/trace.hh as another convenience definition (alongside StringWrap). - Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2279/#review5125 ----------------------------------------------------------- On June 17, 2014, 5:03 p.m., Ali Saidi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2279/ > ----------------------------------------------------------- > > (Updated June 17, 2014, 5:03 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10237:5794a56b79c4 > --------------------------- > cpu: `Minor' in-order CPU model > > This patch contains a new CPU model named `Minor'. Minor models a four > stage in-order execution pipeline (fetch lines, decompose into > macroops, decompose macroops into microops, execute). > > The model was developed to support the ARM ISA but should be fixable > to support all the remaining gem5 ISAs. It currently also works for > Alpha, and regressions are included for ARM and Alpha (including Linux > boot). > > Documentation for the model can be found in src/doc/inside-minor.doxygen and > its internal operations can be visualised using the Minorview tool > utils/minorview.py. > > Minor was designed to be fairly simple and not to engage in a lot of > instruction annotation. As such, it currently has very few gathered > stats and may lack other gem5 features. > > Minor is faster than the o3 model. Sample results: > > Benchmark | Stat host_seconds (s) > ---------------+--------v--------v-------- > (on ARM, opt) | simple | o3 | minor > | timing | timing | timing > ---------------+--------+--------+-------- > 10.linux-boot | 169 | 1883 | 1075 > 10.mcf | 117 | 967 | 491 > 20.parser | 668 | 6315 | 3146 > 30.eon | 542 | 3413 | 2414 > 40.perlbmk | 2339 | 20905 | 11532 > 50.vortex | 122 | 1094 | 588 > 60.bzip2 | 2045 | 18061 | 9662 > 70.twolf | 207 | 2736 | 1036 > > > Diffs > ----- > > build_opts/ALPHA a2bb75a474fd > build_opts/ARM a2bb75a474fd > configs/common/CpuConfig.py a2bb75a474fd > src/base/trace.hh a2bb75a474fd > src/cpu/SConscript a2bb75a474fd > src/cpu/TimingExpr.py PRE-CREATION > src/cpu/minor/MinorCPU.py PRE-CREATION > src/cpu/minor/SConscript PRE-CREATION > src/cpu/minor/SConsopts PRE-CREATION > src/cpu/minor/activity.hh PRE-CREATION > src/cpu/minor/activity.cc PRE-CREATION > src/cpu/minor/cpu.hh PRE-CREATION > src/cpu/minor/cpu.cc PRE-CREATION > src/cpu/minor/decode.hh PRE-CREATION > src/cpu/minor/decode.cc PRE-CREATION > src/cpu/minor/dyn_inst.hh PRE-CREATION > src/cpu/minor/dyn_inst.cc PRE-CREATION > src/cpu/minor/exec_context.hh PRE-CREATION > src/cpu/minor/execute.hh PRE-CREATION > src/cpu/minor/execute.cc PRE-CREATION > src/cpu/minor/fetch1.hh PRE-CREATION > src/cpu/minor/fetch1.cc PRE-CREATION > src/cpu/minor/fetch2.hh PRE-CREATION > src/cpu/minor/fetch2.cc PRE-CREATION > src/cpu/minor/func_unit.hh PRE-CREATION > src/cpu/minor/func_unit.cc PRE-CREATION > src/cpu/minor/lsq.hh PRE-CREATION > src/cpu/minor/lsq.cc PRE-CREATION > src/cpu/minor/pipe_data.hh PRE-CREATION > src/cpu/minor/pipe_data.cc PRE-CREATION > src/cpu/minor/pipeline.hh PRE-CREATION > src/cpu/minor/pipeline.cc PRE-CREATION > src/cpu/minor/scoreboard.hh PRE-CREATION > src/cpu/minor/scoreboard.cc PRE-CREATION > src/cpu/minor/stage.hh PRE-CREATION > src/cpu/minor/stats.hh PRE-CREATION > src/cpu/minor/stats.cc PRE-CREATION > src/cpu/minor/ticked.hh PRE-CREATION > src/cpu/minor/trace.hh PRE-CREATION > src/cpu/pred/SConscript a2bb75a474fd > src/cpu/static_inst.hh a2bb75a474fd > src/cpu/timing_expr.hh PRE-CREATION > src/cpu/timing_expr.cc PRE-CREATION > src/doc/inside-minor.doxygen PRE-CREATION > util/minorview.py PRE-CREATION > util/minorview/__init__.py PRE-CREATION > util/minorview/blobs.py PRE-CREATION > util/minorview/colours.py PRE-CREATION > util/minorview/minor.pic PRE-CREATION > util/minorview/model.py PRE-CREATION > util/minorview/parse.py PRE-CREATION > util/minorview/point.py PRE-CREATION > util/minorview/view.py PRE-CREATION > > Diff: http://reviews.gem5.org/r/2279/diff/ > > > Testing > ------- > > Boots Linux and runs regression tests for ALPHA and ARM. > > > Thanks, > > Ali Saidi > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
