----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/823/#review1476 -----------------------------------------------------------
The way you set up the ReExec fault definitely needs to be fixed before this goes in, but as far as what it's doing it seems basically ok. I don't follow all the details of how you're checking for ordering violations and whether that's right, so I'll leave that to somebody else to check. src/arch/alpha/faults.hh <http://reviews.m5sim.org/r/823/#comment1935> For artificial faults, go ahead and either create a fault in the base files in sim, or perhaps in a file in the O3 directory. The later would probably be better since it doesn't leak O3-ey bits into other parts of the simulator. It can inherit from the base fault classes without involving the ISA at all. You don't need a per-ISA function to generate them since they aren't ISA specific. They're really an implementation detail. In a more generic location it would need a more descriptive/specific/collision resistant name. src/arch/alpha/faults.hh <http://reviews.m5sim.org/r/823/#comment1936> Excessive spacing. src/arch/mips/faults.cc <http://reviews.m5sim.org/r/823/#comment1937> No need for the intermediate variable. Also, I'm not sure what, but you might want to poke some other state since you'll potentially be copying around a structure with a few elements. The trick would be finding something that's safe to poke for all ISAs. src/arch/power/faults.cc <http://reviews.m5sim.org/r/823/#comment1938> I don't think you need three of these... src/arch/x86/faults.hh <http://reviews.m5sim.org/r/823/#comment1939> Inconsistent indentation. This applies elsewhere as well. src/cpu/o3/lsq_impl.hh <http://reviews.m5sim.org/r/823/#comment1940> Not your fault, but else should follow the } src/cpu/o3/lsq_impl.hh <http://reviews.m5sim.org/r/823/#comment1942> How do you expect/did this will affect O3 simulator performance? src/cpu/o3/lsq_impl.hh <http://reviews.m5sim.org/r/823/#comment1941> This is going to create lots of output if this loop goes around a lot. src/cpu/o3/lsq_unit.hh <http://reviews.m5sim.org/r/823/#comment1943> is_load can be determined from inst in checkViolations, can't it? Do we need another parameter? src/cpu/o3/lsq_unit_impl.hh <http://reviews.m5sim.org/r/823/#comment1944> Random whitespace change. Also we probably don't want a straggling blank line at the end of this function. src/cpu/o3/lsq_unit_impl.hh <http://reviews.m5sim.org/r/823/#comment1945> Could the port do this when setPeer is called? src/cpu/o3/lsq_unit_impl.hh <http://reviews.m5sim.org/r/823/#comment1946> spaces around the - src/cpu/o3/lsq_unit_impl.hh <http://reviews.m5sim.org/r/823/#comment1947> This sounds like two sentences squished together. src/cpu/o3/lsq_unit_impl.hh <http://reviews.m5sim.org/r/823/#comment1948> genMachineCheckFault should be the "Oh crap, oh crap, oh crap" fault, not part of normal execution. Ideally it should be eliminated all together, but it definitely shouldn't be built into things like that. - Gabe On 2011-08-18 19:41:19, Ali Saidi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/823/ > ----------------------------------------------------------- > > (Updated 2011-08-18 19:41:19) > > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and > Nathan Binkert. > > > Summary > ------- > > LSQ: Only trigger a memory violation with a load/load if the value changes. > > Only create a memory ordering violation when the value could have changed > between two subsequent loads, instead of just when loads go out-of-order > to the same address. While not very common in the case of Alpha, with > an architecture with a hardware table walker this can happen reasonably > frequently beacuse a translation will miss and start a table walk and > before the CPU re-schedules the faulting instruction another one will > pass it to the same address (or cache block depending on the dependency > checking). > > The performance improvement on SPEC benchmarks can be substantial (2-10%). > > > Diffs > ----- > > src/arch/alpha/faults.hh 7f49e6a176b8 > src/arch/alpha/faults.cc 7f49e6a176b8 > src/arch/arm/faults.hh 7f49e6a176b8 > src/arch/mips/faults.hh 7f49e6a176b8 > src/arch/mips/faults.cc 7f49e6a176b8 > src/arch/power/SConscript 7f49e6a176b8 > src/arch/power/faults.hh 7f49e6a176b8 > src/arch/power/faults.cc PRE-CREATION > src/arch/sparc/faults.hh 7f49e6a176b8 > src/arch/sparc/faults.cc 7f49e6a176b8 > src/arch/x86/faults.hh 7f49e6a176b8 > src/arch/x86/faults.cc 7f49e6a176b8 > src/cpu/base_dyn_inst.hh 7f49e6a176b8 > src/cpu/base_dyn_inst_impl.hh 7f49e6a176b8 > src/cpu/o3/lsq_impl.hh 7f49e6a176b8 > src/cpu/o3/lsq_unit.hh 7f49e6a176b8 > src/cpu/o3/lsq_unit_impl.hh 7f49e6a176b8 > > Diff: http://reviews.m5sim.org/r/823/diff > > > Testing > ------- > > This patch has been tested with a couple of self-checking hand crafted > programs I wrote to stress ordering between two cores. With injected bugs it > definitely shows a failure and it doesn't with this implementation (and it > caught one issue during development). > > > Thanks, > > Ali > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
