> On Sept. 17, 2015, 3:24 p.m., Nilay Vaish wrote: > > src/cpu/o3/probe/elastic_trace.hh, line 304 > > <http://reviews.gem5.org/r/3027/diff/1/?file=48955#file48955line304> > > > > Use a vector here. Ultimately delete everything in one go instead of > > deleting them one by one as you do now.
The patch will be updated soon, just adding the comments now. I have replaced this with a vector and yes it improved the performance 5%-8%. Thanks for the suggestion! > On Sept. 17, 2015, 3:24 p.m., Nilay Vaish wrote: > > src/cpu/o3/probe/elastic_trace.cc, lines 94-97 > > <http://reviews.gem5.org/r/3027/diff/1/?file=48956#file48956line94> > > > > Why do we treat this listener differently from others? Agree this was awkward but I had the consideration of not adding code to python or BaseCPU in mind. The way I've done it now is hopefully cleaner, thanks to Curtis for pointing me to the commit instruction based event queue. > On Sept. 17, 2015, 3:24 p.m., Nilay Vaish wrote: > > src/cpu/o3/probe/elastic_trace.cc, lines 148-154 > > <http://reviews.gem5.org/r/3027/diff/1/?file=48956#file48956line148> > > > > Why would it happen that we receive the execution tick after the > > instruction has been squashed? I came across a special case wherein a retired instruction is propagated back to IEW inst queue, don't remember it and a quick glance of iew_impl.hh didn't help. All I can say is it protects the tempStore from being populated with instructions that are older than lastClearedSeqNum, which had manifested as a memory leak. > On Sept. 17, 2015, 3:24 p.m., Nilay Vaish wrote: > > src/cpu/o3/probe/elastic_trace.cc, line 172 > > <http://reviews.gem5.org/r/3027/diff/1/?file=48956#file48956line172> > > > > std::size_t instead of double? > > > > Can we even do this? Were not statistical variables write-only? Actually you can read the stats using the value(), it saves you one variable. The type is double so if you use anything else you get a compile error. > On Sept. 17, 2015, 3:24 p.m., Nilay Vaish wrote: > > src/cpu/o3/probe/elastic_trace.cc, lines 179-185 > > <http://reviews.gem5.org/r/3027/diff/1/?file=48956#file48956line179> > > > > It is not clear why would this case ever occur. This case occurs when the tracing is configured to start when an instruction count is reached. At that point, all probes start firing. The rename probe point notifies us of the youngest instructions. All other probes are notifying us about older instructions and we don't have these in our temporary store (that stores their timing/dependencies until they commit). > On Sept. 17, 2015, 3:24 p.m., Nilay Vaish wrote: > > src/cpu/o3/probe/elastic_trace.cc, line 219 > > <http://reviews.gem5.org/r/3027/diff/1/?file=48956#file48956line219> > > > > I would rather get an iterator from find() and then make this check to > > avoid having to lookup again. Fixed. > On Sept. 17, 2015, 3:24 p.m., Nilay Vaish wrote: > > src/cpu/o3/probe/elastic_trace.cc, lines 228-229 > > <http://reviews.gem5.org/r/3027/diff/1/?file=48956#file48956line228> > > > > I don't think this check is required since std::set maintains unique > > elements. Fixed. > On Sept. 17, 2015, 3:24 p.m., Nilay Vaish wrote: > > src/cpu/o3/probe/elastic_trace.cc, lines 240-241 > > <http://reviews.gem5.org/r/3027/diff/1/?file=48956#file48956line240> > > > > Why not misc registers? Behaviourally, the execution looks up the misc registers and they are always ready. Therefore I did not model a dependency on misc regs. > On Sept. 17, 2015, 3:24 p.m., Nilay Vaish wrote: > > src/cpu/o3/probe/elastic_trace.cc, lines 253-254 > > <http://reviews.gem5.org/r/3027/diff/1/?file=48956#file48956line253> > > > > I am not sure why you use different types in different places. Again, > > were not statistical values supposed to be write-only? Fixed this to double as well. > On Sept. 17, 2015, 3:24 p.m., Nilay Vaish wrote: > > src/cpu/o3/probe/elastic_trace.cc, lines 262-263 > > <http://reviews.gem5.org/r/3027/diff/1/?file=48956#file48956line262> > > > > First get the iterator, and then perform the check. Then pass on the > > iterator to the erase function. Fixed. > On Sept. 17, 2015, 3:24 p.m., Nilay Vaish wrote: > > src/cpu/o3/probe/elastic_trace.cc, lines 300-308 > > <http://reviews.gem5.org/r/3027/diff/1/?file=48956#file48956line300> > > > > 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? Used an event mechanism instead. > On Sept. 17, 2015, 3:24 p.m., Nilay Vaish wrote: > > src/cpu/o3/probe/elastic_trace.cc, lines 317-326 > > <http://reviews.gem5.org/r/3027/diff/1/?file=48956#file48956line317> > > > > Get the iterator first. Fixed. > On Sept. 17, 2015, 3:24 p.m., Nilay Vaish wrote: > > src/cpu/o3/probe/elastic_trace.cc, line 413 > > <http://reviews.gem5.org/r/3027/diff/1/?file=48956#file48956line413> > > > > This check is not required since the for loop would not execute if the > > set was empty. Fixed. > On Sept. 17, 2015, 3:24 p.m., Nilay Vaish wrote: > > src/cpu/o3/probe/elastic_trace.cc, line 418 > > <http://reviews.gem5.org/r/3027/diff/1/?file=48956#file48956line418> > > > > Get the iterator first and use it for all the processing. Fixed. - Radhika ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3027/#review7201 ----------------------------------------------------------- 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
