o3cpu: lsq: Fix TSO implementation

Information
Submitter:	Marco Elver
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 10149:456195b55e60
---------------------------
o3cpu: lsq: Fix TSO implementation

This patch fixes violation of TSO in the O3CPU, as all loads must be
ordered with all other loads. In the LQ, if a snoop is observed, all
subsequent loads need to be squashed if the system is TSO.

Prior to this patch, the following case could be violated:

 P0         | P1          ;
 MOV [x],$1 | MOV EAX,[y] ;
 MOV [y],$1 | MOV EBX,[x] ;

exists (1:EAX=1 /\ 1:EBX=0) [is a violation]

The problem was found using litmus [http://diy.inria.fr]."

Testing Done

This patch was originally against a much older version of Gem5 (9677:773aae0990ae), where the effect was tested extensively and diy no longer reports violation. This patch was then ported and rebased against the newest Gem5 version, where I only ran quick/se/00.hello/x86/linux/o3-timing. Performance results of parallel workloads will change slightly.

Issue Summary

Description	From	Last Updated	Status
Do you think checking force_squash in this second if() condition will work same as checking needsTSO? If yes, then I ...	Nilay Vaish	March 15, 2014, 12:41 p.m.	Open

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

Do you think checking force_squash in this second if() condition will
work same as checking needsTSO?  If yes, then I would suggest that we
check force_squash here.

Show all issues

Marco Elver March 17, 2014, 7:31 a.m. (March 17, 2014, 7:31 a.m.)

The result would be unaffected only iff (load_addr == invalidate_addr) always implies (ld_inst->possibleLoadViolation()). If this is not the case (and I don't think it is), it is not the same.

If this is a stylistic issue, to preserve the current semantics, I propose: move the if() at 483-485 after the if() at 481 and before the if() at 482; then it is valid to replace the use of needsTSO in the currently second if() with force_squash:

...
        if (load_addr == invalidate_addr || force_squash) {
            if (needsTSO) {
                force_squash = true;
            }
            if (ld_inst->possibleLoadViolation() || force_squash) {
...


The following is also equivalent, but less readable and less flexible (and may lead some to think it is a bug):
...
        if (load_addr == invalidate_addr || force_squash) {
            if (ld_inst->possibleLoadViolation() || (force_squash = needsTSO)) {
...

Change Summary:

Applied suggested change.

Description:

~		Changeset 10101:19483fc4f611
	~	Changeset 10149:456195b55e60

		o3cpu: lsq: Fix TSO implementation

		This patch fixes violation of TSO in the O3CPU, as all loads must be
		ordered with all other loads. In the LQ, if a snoop is observed, all
		subsequent loads need to be squashed if the system is TSO.

		Prior to this patch, the following case could be violated:

		P0 \| P1 ;
		MOV [x],$1 \| MOV EAX,[y] ;
		MOV [y],$1 \| MOV EBX,[x] ;

		exists (1:EAX=1 /\ 1:EBX=0) [is a violation]

~		The problem was found using litmus [http://diy.inria.fr].
	~	The problem was found using litmus [http://diy.inria.fr]."