----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/823/#review1478 -----------------------------------------------------------
src/arch/alpha/faults.hh <http://reviews.m5sim.org/r/823/#comment1972> I initially tried the fault in sim approach, however that doesn't work because the pcstate object is resolved by the namespace and you can't do any template magic to make that happen. There needs to be seperate classes each defined in a namespace FooISA{ ... } or at least using namespace FooISA. While it could be done by doing using namespace TheISA that gets us further away from being able to support a single binary with multiple ISAs, so I don't like it nearly as much. As for the fault, it's a generic fault that could apply to all the CPU models (inorder, out-of-order, any other model) so I think it's more generic than just the o3 even though it's only used here. The fault could also be used in the future to get rid of some of the more specific squashDueToFoo instructions and just use more generic faults to do the job for you. src/arch/alpha/faults.hh <http://reviews.m5sim.org/r/823/#comment1971> fixed src/arch/mips/faults.cc <http://reviews.m5sim.org/r/823/#comment1970> It's not necessary, but I personally like the style better. Do you have an issue with it (the compiler should optimized it away). I can't grok the second half of your statement. src/arch/power/faults.cc <http://reviews.m5sim.org/r/823/#comment1969> dunno how that happened src/arch/x86/faults.hh <http://reviews.m5sim.org/r/823/#comment1959> This is the correct indentation per the style guide src/cpu/o3/lsq_impl.hh <http://reviews.m5sim.org/r/823/#comment1968> fixed src/cpu/o3/lsq_impl.hh <http://reviews.m5sim.org/r/823/#comment1961> Nothing that I can perceive src/cpu/o3/lsq_impl.hh <http://reviews.m5sim.org/r/823/#comment1960> moved out of loop, although it's only called once per thread, so it shouldn't be a big deal src/cpu/o3/lsq_unit.hh <http://reviews.m5sim.org/r/823/#comment1967> removed and replaced with inst->isLoad() src/cpu/o3/lsq_unit_impl.hh <http://reviews.m5sim.org/r/823/#comment1966> fixed src/cpu/o3/lsq_unit_impl.hh <http://reviews.m5sim.org/r/823/#comment1965> There is a potential case where setPeer() is called, but the port doesn't know it's size until the entire memory system is setup (beacuse the peer doesn't know it's size directly and needs to query a peer object that might not yet be connected. This is a more robust implementation. src/cpu/o3/lsq_unit_impl.hh <http://reviews.m5sim.org/r/823/#comment1964> fixed src/cpu/o3/lsq_unit_impl.hh <http://reviews.m5sim.org/r/823/#comment1963> fixed src/cpu/o3/lsq_unit_impl.hh <http://reviews.m5sim.org/r/823/#comment1962> It was used before and it is an oh crap... If we actually commit this instruction we've done something extremely wrong. This is the same way that it was handled previously. - Ali 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
