LSQ: Only trigger a memory violation with a load/load if the value changes.

Information
Submitter:	Ali Saidi
Repository:	gem5
Branch:
Bugs:
Depends On:
Reviewers
Groups:	Default
People:	ali, gblack, nate, stever

Description

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

Testing Done

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

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 (Diff revision 1)

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 (Diff revision 1)

Excessive spacing.

src/arch/mips/faults.cc (Diff revision 1)

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 (Diff revision 1)

I don't think you need three of these...

src/arch/x86/faults.hh (Diff revision 1)

Inconsistent indentation. This applies elsewhere as well.

src/cpu/o3/lsq_impl.hh (Diff revision 1)

Not your fault, but else should follow the }

src/cpu/o3/lsq_impl.hh (Diff revision 1)

How do you expect/did this will affect O3 simulator performance?

src/cpu/o3/lsq_impl.hh (Diff revision 1)

This is going to create lots of output if this loop goes around a lot.

src/cpu/o3/lsq_unit.hh (Diff revision 1)

is_load can be determined from inst in checkViolations, can't it? Do we need another parameter?

src/cpu/o3/lsq_unit_impl.hh (Diff revision 1)

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 (Diff revision 1)

Could the port do this when setPeer is called?

src/cpu/o3/lsq_unit_impl.hh (Diff revision 1)

spaces around the -

src/cpu/o3/lsq_unit_impl.hh (Diff revision 1)

This sounds like two sentences squished together.

src/cpu/o3/lsq_unit_impl.hh (Diff revision 1)

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.

src/arch/alpha/faults.hh (Diff revision 1)

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.

Gabe Black Sept. 2, 2011, 5:33 p.m. (Sept. 2, 2011, 5:33 p.m.)

I don't think TheISA would be a problem, especially if you don't use that temporary since then you wouldn't need to specify the namespace at all. The problem, though, seems to stem from the fact that you're reading and writing the PC. Really all you need to do is write to the threadcontext to convince O3 it needs to flush the pipe, and you can do that by writing anything. You could even add a method which doesn't write anything and just triggers a flush. That also touches on what I was saying a few comments down. If you read the PC you're really creating a local copy of a structure and copying around a chunk of memory, and then when you write it again you put it back. You're not actually moving around useful data, though, so all that copying is wasted effort. If you were to use, say, an int reg, you'd at least only copy around an int.

The problem with using faults more generally is that it presumes that a CPU will care that you're writing to some state. That's really just how O3 works and isn't a requirement at all, so while it might be a useful technique in other CPUs, it's not a generic mechanism. Also, if you're specialized to O3 you can just call functions on the CPU and get things to flush for sure without having to trick the CPU.

Ali Saidi Sept. 3, 2011, 1:42 a.m. (Sept. 3, 2011, 1:42 a.m.)

If that worked, it would be a nice idea (just need to poke the thread context a little bit so that the CPU calls squashFromTC(). However, I went back and looked at the code and it won't work because when the fault is executed inSyscall (the badly named don't cause a fault on an external write flag) is set to true, so external writes won't cause squashes. The only other thing I can think of is to invent yet another mechanism in the O3 CPU to do flushing, but there are more than enough of those already. Actually, I hope that we could move some of the existing ones to attaching faults to instructions over time. Thoughts?

Gabe Black Sept. 4, 2011, 7:59 p.m. (Sept. 4, 2011, 7:59 p.m.)

I thought about this a bit, and actually I don't think you really want to flush. What it seems like you really want to do is to squash all newer instructions once you discover the problem in the LSQ and then get fetch going again from there. Then you don't have to wait for the instruction to make it to the front of the commit queue before getting back on the right path. So even though it would be yet another squash mechanism, a squash mechanism is a better solution than a flushing mechanism. With a fault based flush you have to drain out the whole pipe, but with a squash you'd only have a bubble over part of the pipe.

That said, you're right that the number and diversity of squashing mechanisms in O3 is a bit overwhelming, confusing, error prone, hard to maintain, etc., etc. Rather than not using them, though, it would be best to actually fix that and make them more consistent and generalized. Ideally there would be a single squashing mechanism which would just be applied in each place it was needed, or at least a single system of mechanisms which were all just parameterized or mildly tweaked versions of the same thing, hopefully with lots of code reuse.

Ali Saidi Sept. 5, 2011, 3:03 a.m. (Sept. 5, 2011, 3:03 a.m.)

Yes, another way to handle it is to selectively flush newer instructions. However, the performance benefit of doing so is minimal. When checkSnoop() is called an a ReExec fault occurs the instruction has likely already left the LSQ is is waiting to commit. Dependent instructions have already been issued, could have been executed, and are also likely waiting to commit. Every instruction after that load must be thrown away. Is it possible that a few cycles could be saved by squashing early in this case and letting the fetch stage re-fetch, yes. Would it actually be done in a real processor? Maybe, but not at all guaranteed. There is very little benefit and a huge amount of complexity. The instruction is already going to be in the L1 cache because it just executed, so unlike a branch mispredict, there isn't going to be much in the way of benefit getting fetch to re-direct early. Because the instruction has already executed and is waiting to commit there aren't many cycles until the flush will occur. Finally, It's a very rare case where this fault occurs, unlike the previous implementation that signaled a fault in many cases where it wasn't at all necessary. The proof is that this patch improves performance substantially on ARM and x86 which frequently trip the old mechanism even though they shouldn't because of delayed translations. 

I agree that ideally there would be a single squashing mechanism, but there isn't and that change is outside of the scope of this one.

Gabe Black Sept. 7, 2011, 8:56 p.m. (Sept. 7, 2011, 8:56 p.m.)

Ok, it sounds like you have a good reason to use the fault.

I'm still certain, though, that the fault should not go in each arch. It should either go in sim and use a temporary with TheISA or with no temporary and no TheISA, or in O3 since it makes assumptions about how the CPU works that apply specifically to O3 (and maybe InOrder which barrows a lot of those sorts of things).

src/arch/alpha/faults.hh (Diff revision 1)

fixed

src/arch/mips/faults.cc (Diff revision 1)

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 (Diff revision 1)

dunno how that happened

src/arch/x86/faults.hh (Diff revision 1)

This is the correct indentation per the style guide

Gabe Black Sept. 2, 2011, 5:35 p.m. (Sept. 2, 2011, 5:35 p.m.)

That may be, but it's not consistent with the file. Feel free to fix it, but fix it all at once. Really you shouldn't need to change this file, though, as discussed above.

Ali Saidi Sept. 3, 2011, 1:39 a.m. (Sept. 3, 2011, 1:39 a.m.)
```
Ok
```

src/cpu/o3/lsq_impl.hh (Diff revision 1)

fixed

src/cpu/o3/lsq_impl.hh (Diff revision 1)

Nothing that I can perceive

src/cpu/o3/lsq_impl.hh (Diff revision 1)

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 (Diff revision 1)

removed and replaced with inst->isLoad()

src/cpu/o3/lsq_unit_impl.hh (Diff revision 1)

fixed

src/cpu/o3/lsq_unit_impl.hh (Diff revision 1)

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.

Gabe Black Sept. 2, 2011, 5:35 p.m. (Sept. 2, 2011, 5:35 p.m.)
```
Ok.
```

src/cpu/o3/lsq_unit_impl.hh (Diff revision 1)

fixed

src/cpu/o3/lsq_unit_impl.hh (Diff revision 1)

fixed

src/cpu/o3/lsq_unit_impl.hh (Diff revision 1)

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.

Gabe Black Sept. 2, 2011, 5:35 p.m. (Sept. 2, 2011, 5:35 p.m.)
```
Ok.
```

Diff:

Revision 2 (+150 -41)

Show changes

	src/arch/arm/faults.hh
	src/arch/arm/faults.cc
	src/cpu/base_dyn_inst.hh
	src/cpu/base_dyn_inst_impl.hh
	src/cpu/o3/lsq_impl.hh
	src/cpu/o3/lsq_unit.hh
	src/cpu/o3/lsq_unit_impl.hh
	src/sim/faults.hh
	1 more

This looks good to me. I don't know the LSQ all that well or how your change affects it other than what you have in your description, but I'm willing to assume you got that right. Thanks for moving the fault to sim. I notice you have another fault in ARM which flushes the pipe and also seems fairly generic (modulo the writes == flush assumption). It would be nice if in a future change you moved that to sim too, but leaving it where it is also makes some sense for various reasons.

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Diff: