Give me a chance... I haven't had time to go over Andrew's responses carefully.
I appreciate the changes you've made so far, Andrew. I'm still not convinced that Ticked needs to be a separate class though. What's the point of an interface class when the only use of the interface is in conjunction with a specific implementation? Also, what does minorReport() have to do with evaluate()? It's not clear why these are part of the same interface. All of the methods in TickedModule look like they would be generally useful outside of Minor. Even if existing ClockedModules don't use them, they could migrate to use them eventually. And certainly the TickedModule methods could be useful outside of Minor. If we really think that the few extra fields in TickedModule are a burden, we could just make TickedModule derive from ClockedModule (but still push it up into src/sim). One question I don't know the answer to: are there any ClockedModule instances that would *not* find the TickedModule features useful? I think there is an argument for making these features "standard" across all ClockedModule instances. So how about this proposal: - Get rid of the Ticked interface class, and either (1) integrate the TickedModule features into ClockedModule or (2) add TickedModule as a derived class in src/sim. - Create a MinorModule class that derives from ClockedModule or TickedModule (depending on which way we go above) that adds the virtual minorReport() method. - Everything in Minor that currently derives from TickedModule can derive from MinorModule and won't know the difference. Thoughts? Steve On Tue, Jul 1, 2014 at 7:01 AM, Ali Saidi via gem5-dev <[email protected]> wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2279/#review5171 > ----------------------------------------------------------- > > > Last chance for comments? > > - Ali Saidi > > > 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 > [email protected] > http://m5sim.org/mailman/listinfo/gem5-dev > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
