----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3027/#review7201 -----------------------------------------------------------
This is a very long review, but I think you would some performance improvement if you make the changes I have suggested. src/cpu/o3/probe/elastic_trace.hh (line 304) <http://reviews.gem5.org/r/3027/#comment6120> Use a vector here. Ultimately delete everything in one go instead of deleting them one by one as you do now. src/cpu/o3/probe/elastic_trace.cc (lines 94 - 97) <http://reviews.gem5.org/r/3027/#comment6100> Why do we treat this listener differently from others? src/cpu/o3/probe/elastic_trace.cc (line 123) <http://reviews.gem5.org/r/3027/#comment6101> The function name should be changed since it is only dealing with instruction fetches. src/cpu/o3/probe/elastic_trace.cc (lines 148 - 154) <http://reviews.gem5.org/r/3027/#comment6103> Why would it happen that we receive the execution tick after the instruction has been squashed? src/cpu/o3/probe/elastic_trace.cc (line 162) <http://reviews.gem5.org/r/3027/#comment6102> InstExecInfo *exec_info_ptr; src/cpu/o3/probe/elastic_trace.cc (line 172) <http://reviews.gem5.org/r/3027/#comment6104> std::size_t instead of double? Can we even do this? Were not statistical variables write-only? src/cpu/o3/probe/elastic_trace.cc (lines 179 - 185) <http://reviews.gem5.org/r/3027/#comment6105> It is not clear why would this case ever occur. src/cpu/o3/probe/elastic_trace.cc (line 216) <http://reviews.gem5.org/r/3027/#comment6106> PhysRegIndex instead of short int. src/cpu/o3/probe/elastic_trace.cc (line 219) <http://reviews.gem5.org/r/3027/#comment6107> I would rather get an iterator from find() and then make this check to avoid having to lookup again. src/cpu/o3/probe/elastic_trace.cc (lines 228 - 229) <http://reviews.gem5.org/r/3027/#comment6108> I don't think this check is required since std::set maintains unique elements. src/cpu/o3/probe/elastic_trace.cc (lines 240 - 241) <http://reviews.gem5.org/r/3027/#comment6110> Why not misc registers? src/cpu/o3/probe/elastic_trace.cc (line 242) <http://reviews.gem5.org/r/3027/#comment6109> This is index type that you should use in other places as well. src/cpu/o3/probe/elastic_trace.cc (lines 253 - 254) <http://reviews.gem5.org/r/3027/#comment6111> I am not sure why you use different types in different places. Again, were not statistical values supposed to be write-only? src/cpu/o3/probe/elastic_trace.cc (lines 262 - 263) <http://reviews.gem5.org/r/3027/#comment6112> First get the iterator, and then perform the check. Then pass on the iterator to the erase function. src/cpu/o3/probe/elastic_trace.cc (lines 272 - 273) <http://reviews.gem5.org/r/3027/#comment6113> This pattern of first getting the iterator and then processing, should be used in other places as well. src/cpu/o3/probe/elastic_trace.cc (line 281) <http://reviews.gem5.org/r/3027/#comment6114> <type> *, not <type>* src/cpu/o3/probe/elastic_trace.cc (lines 300 - 308) <http://reviews.gem5.org/r/3027/#comment6115> This just seems to be really arbitrary place to have this code. Can we have some event that happens when the numSimulatedInsts has reached a certain value? src/cpu/o3/probe/elastic_trace.cc (lines 317 - 326) <http://reviews.gem5.org/r/3027/#comment6117> Get the iterator first. src/cpu/o3/probe/elastic_trace.cc (line 413) <http://reviews.gem5.org/r/3027/#comment6118> This check is not required since the for loop would not execute if the set was empty. src/cpu/o3/probe/elastic_trace.cc (line 418) <http://reviews.gem5.org/r/3027/#comment6119> Get the iterator first and use it for all the processing. - Nilay Vaish On Aug. 11, 2015, 9:05 p.m., Curtis Dunham wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3027/ > ----------------------------------------------------------- > > (Updated Aug. 11, 2015, 9:05 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > The elastic trace is a type of probe listener and listens to probe points > in multiple stages of the O3CPU. The notify method is called on a probe > point typically when an instruction successfully progresses through that > stage. > > As different listener methods mapped to the different probe points execute, > relevant information about the instruction, e.g. timestamps and register > accesses, are captured and stored in temporary InstExecInfo class objects. > When the instruction progresses through the commit stage, the timing and the > dependency information about the instruction is finalised and encapsulated in > a struct called TraceInfo. TraceInfo objects are collected in a list instead > of writing them out to the trace file one a time. This is required as the > trace is processed in chunks to evaluate order dependencies and computational > delay in case an instruction does not have any register dependencies. By this > we achieve a simpler algorithm during replay because every record in the > trace can be hooked onto a record in its past. The instruction dependency > trace is written out as a protobuf format file. A second trace containing > fetch requests at absolute timestamps is written to a separate protobuf > format file. > > If the instruction is not executed then it is not added to the trace. > The code checks if the instruction had a fault, if it predicated > false and thus previous register values were restored or if it was a > load/store that did not have a request (e.g. when the size of the > request is zero). In all these cases the instruction is set as > executed by the Execute stage and is picked up by the commit probe > listener. But a request is not issued and registers are not written. > So practically, skipping these should not hurt the dependency modelling. > > If squashing results in squashing younger instructions, it may happen that > the squash probe discards the inst and removes it from the temporary > store but execute stage deals with the instruction in the next cycle which > results in the execute probe seeing this inst as 'new' inst. A sequence > number of the last processed trace record is used to trap these cases and > not add to the temporary store. > > The elastic instruction trace and fetch request trace can be read in and > played back by the TraceCPU. > > > Diffs > ----- > > src/cpu/o3/probe/ElasticTrace.py PRE-CREATION > src/cpu/o3/probe/SConscript b998b5a6c5f59b41e0c0997ca1bebe37717ad551 > src/cpu/o3/probe/elastic_trace.hh PRE-CREATION > src/cpu/o3/probe/elastic_trace.cc PRE-CREATION > src/proto/SConscript b998b5a6c5f59b41e0c0997ca1bebe37717ad551 > src/proto/inst_dep_record.proto PRE-CREATION > src/proto/packet.proto b998b5a6c5f59b41e0c0997ca1bebe37717ad551 > > Diff: http://reviews.gem5.org/r/3027/diff/ > > > Testing > ------- > > > Thanks, > > Curtis Dunham > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
