----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2279/#review5125 -----------------------------------------------------------
Nice! I didn't have time to read all the code closely, but I did notice a few things, and wanted to give feedback before I forgot. Basically the comments boil down to three things: 1. There are several pieces (Named, TickedModule, maybe the Expr stuff) that look more broadly useful, and it would be nice not to bury them inside of the CPU model 2. The report() functionality, at least, looks similar to methods on other objects like dump() and print(); we might want to standardize on this 3. A few minor (no pun intended) places where there are formatting inconsistencies with undocumented gem5 conventions As far as #3, I'd be glad to update the coding style page appropriately if people agree that the things I've called out below should be standardized. Where does the name come from? Will you be committing a 'Major' model in the future? src/cpu/minor/Expr.py <http://reviews.gem5.org/r/2279/#comment4643> This is pretty interesting... it would be nice to generalize this capability and not make it Minor-specific src/cpu/minor/cpu.hh <http://reviews.gem5.org/r/2279/#comment4634> I don't know why, and I know it's not in the style guide, but all the gem5 code I've seen (or written) has the colon at the beginning of the second line, not the end of the first... might as well be consistent src/cpu/minor/decode.cc <http://reviews.gem5.org/r/2279/#comment4635> another implicit, undocumented consistency thing: continued arg lists get indented to the opening paren, not just four spaces src/cpu/minor/dyn_inst.hh <http://reviews.gem5.org/r/2279/#comment4636> I'd put a paren at the beginning of this expression, then indent so all the lines start one column past the paren... keeps the assignment '=' from blending in with all the '==' src/cpu/minor/scoreboard.cc <http://reviews.gem5.org/r/2279/#comment4637> space after for src/cpu/minor/scoreboard.cc <http://reviews.gem5.org/r/2279/#comment4638> opening brace at end of previous line (here & a few places below) src/cpu/minor/ticked.hh <http://reviews.gem5.org/r/2279/#comment4639> 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 src/cpu/minor/ticked.hh <http://reviews.gem5.org/r/2279/#comment4640> We also have various objects that have dump() or print() methods... it would be nice to standardize this src/cpu/minor/ticked.hh <http://reviews.gem5.org/r/2279/#comment4641> integrating with ClockedObject would eliminate the multiple inheritance... not that MI is totally evil, but it's always better to avoid it when possible src/cpu/minor/trace.hh <http://reviews.gem5.org/r/2279/#comment4642> This would be very useful outside of Minor... should go in src/base/trace.hh, IMO src/cpu/static_inst.hh <http://reviews.gem5.org/r/2279/#comment4633> blank line here would be nice - Steve Reinhardt On May 30, 2014, 8:23 a.m., Ali Saidi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2279/ > ----------------------------------------------------------- > > (Updated May 30, 2014, 8:23 a.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/cpu/minor/Expr.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/expr.hh PRE-CREATION > src/cpu/minor/expr.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/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 [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
