O3: Tighten memory order violation checking to 16 bytes.

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

Description

O3: Tighten memory order violation checking to 16 bytes.

The comment in the code suggests that the checking granularity should be 16
bytes, however in reality the shift by 8 is 256 bytes which seems much
larger than required.

Testing Done

Should we put an assert in there to make sure no access is bigger than 16 bytes? Also what about unaligned accesses? I think those will be split on cache block boundaries which may be bigger or smaller than 16 bytes. We might have an access that spans from one 16 byte chunk to the next. These aren't really problems with this change, but it might make them easier to hit.

I'm assuming this had some effect on the regressions. Did things generally go faster, slower, etc.?

Ali Saidi March 17, 2011, 10:27 a.m. (March 17, 2011, 10:27 a.m.)
```
Not major changes, but things usually sped up a little bit.
```

Ali Saidi March 18, 2011, 12:22 a.m. (March 18, 2011, 12:22 a.m.)

Actually it is quite a change for some benchmarks, it's just not uniform. High of 17%, low of 0%, average of about 5%. So I think we need to fix it and if it exposes other bugs, so be it, but the regression tests all complete successfully. 

alpha
------------------
eon: 17%
twolf: 15%
vortex: 5%
gzip 0%
linux: 0%
bzip2: 3%
perlbmk: 2%


parc
--------------------
gzip: 3%

x86
--------------------
mcf: 17%
parser: 4%
twolf: 2%
gzip: 2%

arm
----------------------
gzip: 7%
mcf: 2%
eon: 13%
twolf: 3%
parser: 3%
vortex: 6%
perlbmk: 
bzip2:
linux: 1%

Gabe Black March 18, 2011, 7:19 a.m. (March 18, 2011, 7:19 a.m.)

Yes, we shouldn't delay fixing this bug for the sake of the other ones that might be in there too. We should still add an assert if possible, though, so we catch those bugs if they happen.

Diff:

Revision 2 (+81 -69)

Show changes

	src/cpu/base_dyn_inst.hh
	src/cpu/o3/O3CPU.py
	src/cpu/o3/lsq_unit.hh
	src/cpu/o3/lsq_unit_impl.hh

I think the updated patch addresses all of your issues Gabe. I tested it with an opt binary and one problem jumped out in x86 for 20.parser an assert: 
m5.opt: build/X86_SE/arch/x86/emulenv.cc:49: void X86ISA::EmulEnv::doModRM(const X86ISA::ExtMachInst&): Assertion `machInst.modRM.mod != 3' failed.

It looks like the assert shouldn't be there and is hit during some miss speculation.

I can't verify 100% that the code in your new function is correct, but I don't see anything obviously wrong. I really like that you consolidated the same code in two places down to the one. There's one issue which is pointed out below.

src/cpu/base_dyn_inst.hh (Diff revision 2)

This comment is inaccurate. It's really the largest address that's part of the request, which is the effective address plus the size and then minus one. Also, this feels like a temporary variable promoted to too large of a scope and/or permanence. "size" seems like it would be more generally useful, it would be more immediately obvious what it is, and you can go from one to the other easily like you are elsewhere in this change.

Ali Saidi March 30, 2011, 4:40 a.m. (March 30, 2011, 4:40 a.m.)
```
I'll change it to size.
```

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Diff: