[PATCH v2, RTL]: Fix PR 63483, Scheduler performs Invalid move of aliased memory reference
On Wed, Oct 8, 2014 at 1:56 PM, Uros Bizjak ubiz...@gmail.com wrote:
This message revives an old thread [1], where the miscompilation of gfortran
on alpha was found that that resulted in:
[...]
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.
Attached v2 patch implements the same approach in all alias.c places
that declare MEM_READONLY_P operands as never aliasing.
2014-10-10 Uros Bizjak ubiz...@gmail.com
* alias.c (true_dependence_1): Do not exit early for MEM_READONLY_P
references when alignment ANDs are involved.
(write_dependence_p): Ditto.
(may_alias_p): Ditto.
Patch was boostrapped and regression tested on x86_64-linux-gnu and
alpha-linux-gnu. Unfortunately, there are still failures remaining in
gfortran testsuite due to independent RTL infrastructure problem with
VALUEs leaking into aliasing detecting functions [2], [3].
The patch was discussed and OK'd by Richi in the PR audit trail. If
there are no objections from RTL maintainers, I plan to commit it to
the mainline early next week.
[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63483
[2] https://gcc.gnu.org/ml/gcc/2014-10/msg00060.html
[3] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63475
Uros.
Index: alias.c
===
--- alias.c (revision 216025)
+++ alias.c (working copy)
@@ -2458,18 +2458,6 @@ true_dependence_1 (const_rtx mem, enum machine_mod
|| MEM_ALIAS_SET (mem) == ALIAS_SET_MEMORY_BARRIER)
return 1;
- /* Read-only memory is by definition never modified, and therefore can't
- conflict with anything. We don't expect to find read-only set on MEM,
- but stupid user tricks can produce them, so don't die. */
- if (MEM_READONLY_P (x))
-return 0;
-
- /* If we have MEMs referring to different address spaces (which can
- potentially overlap), we cannot easily tell from the addresses
- whether the references overlap. */
- if (MEM_ADDR_SPACE (mem) != MEM_ADDR_SPACE (x))
-return 1;
-
if (! mem_addr)
{
mem_addr = XEXP (mem, 0);
@@ -2493,6 +2481,22 @@ true_dependence_1 (const_rtx mem, enum machine_mod
}
}
+ /* Read-only memory is by definition never modified, and therefore can't
+ conflict with anything. However, don't assume anything when AND
+ addresses are involved and leave to the code below to determine
+ dependence. We don't expect to find read-only set on MEM, but
+ stupid user tricks can produce them, so don't die. */
+ if (MEM_READONLY_P (x)
+ GET_CODE (x_addr) != AND
+ GET_CODE (mem_addr) != AND)
+return 0;
+
+ /* If we have MEMs referring to different address spaces (which can
+ potentially overlap), we cannot easily tell from the addresses
+ whether the references overlap. */
+ if (MEM_ADDR_SPACE (mem) != MEM_ADDR_SPACE (x))
+return 1;
+
base = find_base_term (x_addr);
if (base (GET_CODE (base) == LABEL_REF
|| (GET_CODE (base) == SYMBOL_REF
@@ -2576,16 +2580,6 @@ write_dependence_p (const_rtx mem,
|| MEM_ALIAS_SET (mem) == ALIAS_SET_MEMORY_BARRIER)
return 1;
- /* A read from read-only memory can't conflict with read-write memory. */
- if (!writep MEM_READONLY_P (mem))
-return 0;
-
- /* If we have MEMs referring to different address spaces (which can
- potentially overlap), we cannot easily tell from the addresses
- whether the references overlap. */
- if (MEM_ADDR_SPACE (mem) != MEM_ADDR_SPACE (x))
-return 1;
-
mem_addr = XEXP (mem, 0);
if (!x_addr)
{
@@ -2603,6 +2597,21 @@ write_dependence_p (const_rtx mem,
}
}
+ /* A read from read-only memory can't conflict with read-write memory.
+ Don't assume anything when AND addresses are involved and leave to
+ the code below to determine dependence. */
+ if (!writep
+ MEM_READONLY_P (mem)
+ GET_CODE (x_addr) != AND
+ GET_CODE (mem_addr) != AND)
+return 0;
+
+ /* If we have MEMs referring to different address spaces (which can
+ potentially overlap), we cannot easily tell from the addresses
+ whether the references overlap. */
+ if (MEM_ADDR_SPACE (mem) != MEM_ADDR_SPACE (x))
+return 1;
+
base = find_base_term (mem_addr);
if (! writep
base
@@ -2690,18 +2699,6 @@ may_alias_p (const_rtx mem, const_rtx x)
|| MEM_ALIAS_SET (mem) == ALIAS_SET_MEMORY_BARRIER)
return 1;
- /* Read-only memory is by definition never modified, and therefore can't
- conflict with anything. We don't expect to find read-only set on MEM,
- but stupid user tricks can
Re: [PATCH v2, RTL]: Fix PR 63483, Scheduler performs Invalid move of aliased memory reference
On 10/10/14 04:06, Uros Bizjak wrote: On Wed, Oct 8, 2014 at 1:56 PM, Uros Bizjak ubiz...@gmail.com wrote: This message revives an old thread [1], where the miscompilation of gfortran on alpha was found that that resulted in: [...] 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. Attached v2 patch implements the same approach in all alias.c places that declare MEM_READONLY_P operands as never aliasing. 2014-10-10 Uros Bizjak ubiz...@gmail.com * alias.c (true_dependence_1): Do not exit early for MEM_READONLY_P references when alignment ANDs are involved. (write_dependence_p): Ditto. (may_alias_p): Ditto. Patch was boostrapped and regression tested on x86_64-linux-gnu and alpha-linux-gnu. Unfortunately, there are still failures remaining in gfortran testsuite due to independent RTL infrastructure problem with VALUEs leaking into aliasing detecting functions [2], [3]. The patch was discussed and OK'd by Richi in the PR audit trail. If there are no objections from RTL maintainers, I plan to commit it to the mainline early next week. [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63483 [2] https://gcc.gnu.org/ml/gcc/2014-10/msg00060.html [3] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63475 No objection. In fact, after reading everything it's pretty obvious to me that a /u MEM must be considered as potentially conflicting with writes that are implemented as RMW sequences to deal with the lack of byte access support. The escaping VALUE stuff is still in my queue. jeff
Re: [PATCH v2, RTL]: Fix PR 63483, Scheduler performs Invalid move of aliased memory reference
On Fri, Oct 10, 2014 at 7:25 PM, Jeff Law l...@redhat.com wrote: This message revives an old thread [1], where the miscompilation of gfortran on alpha was found that that resulted in: [...] 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. Attached v2 patch implements the same approach in all alias.c places that declare MEM_READONLY_P operands as never aliasing. 2014-10-10 Uros Bizjak ubiz...@gmail.com * alias.c (true_dependence_1): Do not exit early for MEM_READONLY_P references when alignment ANDs are involved. (write_dependence_p): Ditto. (may_alias_p): Ditto. Patch was boostrapped and regression tested on x86_64-linux-gnu and alpha-linux-gnu. Unfortunately, there are still failures remaining in gfortran testsuite due to independent RTL infrastructure problem with VALUEs leaking into aliasing detecting functions [2], [3]. The patch was discussed and OK'd by Richi in the PR audit trail. If there are no objections from RTL maintainers, I plan to commit it to the mainline early next week. [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63483 [2] https://gcc.gnu.org/ml/gcc/2014-10/msg00060.html [3] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63475 No objection. In fact, after reading everything it's pretty obvious to me that a /u MEM must be considered as potentially conflicting with writes that are implemented as RMW sequences to deal with the lack of byte access support. Thanks, I went ahead and commit the patch to SVN mainline. I wonder, if they should be also committed to release branches? The escaping VALUE stuff is still in my queue. Great, I can test them on alpha native, there are many gfortran testsuite failures due to this problem. Thanks, Uros.
Re: [PATCH v2, RTL]: Fix PR 63483, Scheduler performs Invalid move of aliased memory reference
On 10/10/14 11:39, Uros Bizjak wrote: No objection. In fact, after reading everything it's pretty obvious to me that a /u MEM must be considered as potentially conflicting with writes that are implemented as RMW sequences to deal with the lack of byte access support. Thanks, I went ahead and commit the patch to SVN mainline. I wonder, if they should be also committed to release branches? Do we have anything other than the ancient alphas that don't have byte access and thus have to generate the RMW sequences? If it's just the ancient alphas, I wouldn't bother. jeff
