Yep, not needed - you won't see this in the updated patch.

Agree. Created a new example file for trace replay - hopefully setting a good example. :)

Adopted both your suggestions.

Fixed.

Fixed.

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.

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.

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.

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.

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...

If you want to add this script, then along with the script, add a regression test that uses it.

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.

The plan is to merge all the elastic traces patches and commit in a single changeset.

Do you mean from reviews 3026-3031 are going to be committed as one single changeset?

Is it required due to some dependency?  I somehow prefer the current division into

5/6 different patches.

Hi Nilay, I had a chat with folks here and realised I was under the wrong impression. Yes, the patches they way they are organised will be committed. There are 8 in total and you have reviewed 6 of them. Does the division for the 6 seem reasonable? Each patch adds a new independent functionality applies and passess tests individually.

Yes, merging the trace_cpu.cc changes into the patch that adds the Trace CPU.