> On June 4, 2014, 8:15 p.m., Steve Reinhardt wrote: > > 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? > >
I'll make separate replies for each of your points but here's the philosophy: 1. I'll address these one by one as the arguments are a bit different for each 2. Report probably has too specific a name, I don't think it fits with the dump/print pattern as well as it may appear. I'll off more reasoning below 3. I'll fix/comment. I only have one personal strong style dislike to express (== plea to not standardise). Oh, the name. M(odel|icroprocessor)INORder. The model has gone through a few names but that was the one that seemed to stick. Unfortunately it doesn't really pass the tell-apart-from-a-dictionary-word test and I find myself having to say 'Minor CPU' a bit too much, so it's not absolutely ideal. There are no plans for a Major. Sorry to raise hopes ;) > 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 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? > On June 4, 2014, 8:15 p.m., Steve Reinhardt wrote: > > src/cpu/minor/cpu.hh, line 88 > > <http://reviews.gem5.org/r/2279/diff/1/?file=39833#file39833line88> > > > > 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 I've run a couple of regexps over the codebase (quick and dirty, not checked all the matches): '^ * : [a-z]' (next line :) 257 lines '\) :'$ (trailing :) 394 lines If you drop uses in src/arch/... (as the archs seem to have slightly more fast and loose enforcement of the style rules): next line: 231 lines trailing: 125 lines So next line seems to be winning. I have no real preference. Do you want to add this to the style guide? > On June 4, 2014, 8:15 p.m., Steve Reinhardt wrote: > > src/cpu/minor/decode.cc, line 153 > > <http://reviews.gem5.org/r/2279/diff/1/?file=39836#file39836line153> > > > > another implicit, undocumented consistency thing: continued arg lists > > get indented to the opening paren, not just four spaces Erm, yeah, I really strongly disagree with hanging arguments. I find they waste too much space (and break my indentation fascist person rule about always indenting exactly once and always the same size, but that's my psychological problem to deal with ;) ). I throw myself of the mercy of the style enforcement court and plea for hanging arguments *not* become an explicit style rule. > On June 4, 2014, 8:15 p.m., Steve Reinhardt wrote: > > src/cpu/minor/dyn_inst.hh, line 126 > > <http://reviews.gem5.org/r/2279/diff/1/?file=39837#file39837line126> > > > > 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 '==' Yeah, that line could be *much* clearer. I've put parens around the whole expression. Are parens around equality/inequality tests preferred as well? I've seen cases in the codebase. > On June 4, 2014, 8:15 p.m., Steve Reinhardt wrote: > > src/cpu/minor/scoreboard.cc, line 164 > > <http://reviews.gem5.org/r/2279/diff/1/?file=39857#file39857line164> > > > > space after for Yep > On June 4, 2014, 8:15 p.m., Steve Reinhardt wrote: > > src/cpu/minor/scoreboard.cc, line 238 > > <http://reviews.gem5.org/r/2279/diff/1/?file=39857#file39857line238> > > > > opening brace at end of previous line (here & a few places below) There are lots of those. They're following the explicitly allowable case where the condition is longer than a line. There may be some wild cases left where a modified condition *does* now fit on the line but may still be split (for unjustifiable reasons) but I've spent quite a bit of time cleaning those before the patch was submitted. > 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 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. > 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 I think I've answered that one above. > 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 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) > 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 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? > On June 4, 2014, 8:15 p.m., Steve Reinhardt wrote: > > src/cpu/static_inst.hh, line 66 > > <http://reviews.gem5.org/r/2279/diff/1/?file=39864#file39864line66> > > > > blank line here would be nice Before SymbolTable? Yes. A small style question: What's should be the preferred form for namespaces? I've seen both brace location options used and I've gone with next-line braces for all uses of namespaces as they are large scale structures. The use of namespace in static_inst.hh is really just a fancy (and unfortunately required) way of building a reference to a class, should putting the brace on the same line be the preferred form for this one-liner? (As with Trace::InstRecord). If this isn't already in the style guide, maybe it should be. - Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2279/#review5125 ----------------------------------------------------------- On May 30, 2014, 3:23 p.m., Ali Saidi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2279/ > ----------------------------------------------------------- > > (Updated May 30, 2014, 3:23 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/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
