> On Nov. 6, 2015, 7:50 a.m., Nilay Vaish wrote: > > configs/example/etrace_replay.py, lines 107-109 > > <http://reviews.gem5.org/r/3031/diff/2/?file=51462#file51462line107> > > > > I think these are the only lines that extra > > over what se.py has. Why not just move these to se.py? > > Radhika Jagtap wrote: > There were a few code blocks where cpu or workload was being configured > for se and fs. That code did not apply for a trace-based cpu. So, in the > previous version of this patch I had inserted a check a all these points - if > cpu type is not trace then do these code blocks else do trace cpu counterpart > or do nothing. > > Nilay Vaish wrote: > I saw the first version of the patch. I am actually fine with adding a > check in se.py and > assiging these two trace file variables. Since the TraceCPU inherits > from BaseCPU, you > do not even need to worry about some code being not applicable to the > TraceCPU since the > BaseCPU class will take care of it. It is sort of similar to when we > start from a checkpoint > we still need to specify everything. > > Radhika Jagtap wrote: > In Simulation.py the block of code which assigns max insts per thread, > workload, checker etc. to switch_cpus ends up having 4 levels of indentation > which I'm keen on avoiding. Trace replay example script should be a separate > file because it is short and simple, and barely any code is replicated from > se and fs. As there is no need to test if cpu type is trace in every other > block of configuration statements (which was required in the previous > version), the indentation level is at most 1, which I think is a good example > script. > > Nilay Vaish wrote: > >In Simulation.py the block of code which assigns max insts per thread, > workload, checker etc. to switch_cpus ends up having 4 levels of indentation > which I'm keen on avoiding. Trace replay example script should be a separate > file because it is short and simple, and barely any code is replicated from > se and fs. As there is no need to test if cpu type is trace in every other > block of configuration statements (which was required in the previous > version), the indentation level is at most 1, which I think is a good example > script. > > > Since the etrace_replay script calls Simulation.run(), I hardly think > that whatever you wrote > above is right. Other than se.py and fs.py, we do not maintain the > scripts in configs/example that well. > So I do not like adding this new script at all. > > Andreas Hansson wrote: > I must say I really don't like the "normal" route of jamming everything > into se.py and fs.py. We should rather try and make the scripts simple and > understandable. > > It's far easier to maintain two simple scripts than one complex one with > an intricate control flow... > > Nilay Vaish wrote: > If you want to add this script, then along with the script, add a > regression test that uses it. > > Andreas Hansson wrote: > I agree that it is desirable, but I am not sure that makes sense in this > instance. We'd have to also add traces to the repo, further bloating it with > data that shouldn't really be there.
Two things. First, I think it is possible that the regression test first creates a trace and then uses it. Second, as far as just maintaining the script is concerned, an empty trace file / a very short trace would be sufficient. - Nilay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3031/#review7520 ----------------------------------------------------------- On Dec. 3, 2015, 1:34 a.m., Curtis Dunham wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3031/ > ----------------------------------------------------------- > > (Updated Dec. 3, 2015, 1:34 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > This patch adds changes to the configuration scripts to support elastic > tracing and replay. > > The patch adds a command line option to enable elastic tracing in SE mode > and FS mode. When enabled the Elastic Trace cpu probe is attached to O3CPU > and a few O3 CPU parameters are tuned. The Elastic Trace probe writes out > both instruction fetch and data dependency traces. The patch also enables > configuring the TraceCPU to replay traces using the SE and FS script. > > The replay run is designed to resume from checkpoint using atomic cpu to > restore state keeping it consistent with FS run flow. It then switches to > TraceCPU to replay the input traces. > > > Diffs > ----- > > configs/common/CacheConfig.py 135c16fa409d814dfbda979e59583534601dc1c7 > configs/common/CpuConfig.py 135c16fa409d814dfbda979e59583534601dc1c7 > configs/common/MemConfig.py 135c16fa409d814dfbda979e59583534601dc1c7 > configs/common/Options.py 135c16fa409d814dfbda979e59583534601dc1c7 > configs/common/Simulation.py 135c16fa409d814dfbda979e59583534601dc1c7 > configs/dram/sweep.py 135c16fa409d814dfbda979e59583534601dc1c7 > configs/example/etrace_replay.py PRE-CREATION > configs/example/fs.py 135c16fa409d814dfbda979e59583534601dc1c7 > configs/example/se.py 135c16fa409d814dfbda979e59583534601dc1c7 > > Diff: http://reviews.gem5.org/r/3031/diff/ > > > Testing > ------- > > > Thanks, > > Curtis Dunham > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
