[PATCH, RTL]: Fix PR 63483, Scheduler performs Invalid move of aliased memory reference

[Uros Bizjak]

Hello!

This message revives an old thread [1], where the miscompilation of gfortran
on alpha was found that that resulted in:

FAIL: gfortran.dg/assumed_rank_3.f90:15.20:

print *, ubound(x,dim=3) ! << wrong dim
1
Error: Assumed-rank variable x at (1) may only be used as actual argument

The problem was in the miscompilation of resolve_actual_arglist from
resolve.c. This function initializes two nearby global bool variables
with the following sequence:

actual_arg = true;
first_actual_arg = true;

but due to the miscompilation, the actual_arg was never set to true.
This happened due to the way stores to QImode and HImode locations are
implemented on non-BWX targets. The sequence reads full word, does its
magic to the part and stores the full word with changed part back to
the memory. However - the scheduler mixed two sequences, violating the
atomicity of RMW sequence.

As demostrated in the great detail in the PR [2], the problem is in
early exit for MEM_READOLNY_P in true_dependence_1 in alias.c. This
early exit declares all MEM_READONLY_P references as non-aliasing,
which is not true when possibly aliasing references with alignment
ANDs are involved.

Proposed patch prevents MEM_READONLY_P memory references to be moved
over possibly aliased memory (with alignment ANDs). The patch prevents
early exit for MEM_READONLY_P references when alignment ANDs are
involved. The aliasing is then determined later in the function. In
effect, it changes early exit to:

- if (MEM_READONLY_P (x))
- return 0;
+ if (MEM_READONLY_P (x)
+ && GET_CODE (x_addr) != AND
+ && GET_CODE (mem_addr) != AND)
+ return 0;

The comment also mentions "... We don't expect to find read-only set
on MEM, but stupid user tricks can produce them, so don't die.". We
certainly don't die anymore, as confirmed by a native alpha-linux-gnu
(please note - not alphaev6!) bootstrap and regression test.

2014-10-08 Uros Bizjak <***@gmail.com>

* alias.c (true_dependence_1): Do not exit early for MEM_READONLY_P
references when alignment ANDs are involved.

The patch was bootstrapped and regression tested on alpha-linux-gnu.

OK for mainline and release branches?

Please note that the patch by itself is not enough to fix the original
problem with gfortran miscompilation. Another problem in this area is
summarised in PR 63475 [3], where postreload CSE propagates aliased
memory operand.

[1] https://gcc.gnu.org/ml/gcc-patches/2014-06/msg02251.html
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63483
[3] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63475

Uros.

[Richard Biener]

Post by Uros Bizjak
Hello!
This message revives an old thread [1], where the miscompilation of gfortran
print *, ubound(x,dim=3) ! << wrong dim
1
Error: Assumed-rank variable x at (1) may only be used as actual argument
The problem was in the miscompilation of resolve_actual_arglist from
resolve.c. This function initializes two nearby global bool variables
actual_arg = true;
first_actual_arg = true;
but due to the miscompilation, the actual_arg was never set to true.
This happened due to the way stores to QImode and HImode locations are
implemented on non-BWX targets. The sequence reads full word, does its
magic to the part and stores the full word with changed part back to
the memory. However - the scheduler mixed two sequences, violating the
atomicity of RMW sequence.
As demostrated in the great detail in the PR [2], the problem is in
early exit for MEM_READOLNY_P in true_dependence_1 in alias.c. This
early exit declares all MEM_READONLY_P references as non-aliasing,
which is not true when possibly aliasing references with alignment
ANDs are involved.
Proposed patch prevents MEM_READONLY_P memory references to be moved
over possibly aliased memory (with alignment ANDs). The patch prevents
early exit for MEM_READONLY_P references when alignment ANDs are
involved. The aliasing is then determined later in the function. In
- if (MEM_READONLY_P (x))
- return 0;
+ if (MEM_READONLY_P (x)
+ && GET_CODE (x_addr) != AND
+ && GET_CODE (mem_addr) != AND)
+ return 0;
The comment also mentions "... We don't expect to find read-only set
on MEM, but stupid user tricks can produce them, so don't die.". We
certainly don't die anymore, as confirmed by a native alpha-linux-gnu
(please note - not alphaev6!) bootstrap and regression test.
* alias.c (true_dependence_1): Do not exit early for MEM_READONLY_P
references when alignment ANDs are involved.
The patch was bootstrapped and regression tested on alpha-linux-gnu.
OK for mainline and release branches?

As said in the audit trail of the bugreport I think that the caller
of alpha_set_memflags is wrong in applying MEM flags from the _source_
operand to MEMs generated for the RMW sequence for the destination.

It would be better to fix that instead.

Richard.

Post by Uros Bizjak
Please note that the patch by itself is not enough to fix the original
problem with gfortran miscompilation. Another problem in this area is
summarised in PR 63475 [3], where postreload CSE propagates aliased
memory operand.
[1] https://gcc.gnu.org/ml/gcc-patches/2014-06/msg02251.html
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63483
[3] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63475
Uros.

[Uros Bizjak]

Post by Richard Biener

Post by Uros Bizjak
This message revives an old thread [1], where the miscompilation of gfortran

[...]

Post by Richard Biener
As said in the audit trail of the bugreport I think that the caller
of alpha_set_memflags is wrong in applying MEM flags from the _source_
operand to MEMs generated for the RMW sequence for the destination.
It would be better to fix that instead.

Please see comment #6 of the referred PR [1] for further analysis and
ammended testcase. The testcase and analysis will show a "native" read
passing possibly aliasing store.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63483#c6

Uros.