[Bug target/57341] [4.8/4.9 Regression] wrong code on x86_64-linux at -O3 in 32-bit mode
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57341 Richard Biener changed: What|Removed |Added Status|ASSIGNED|RESOLVED Resolution|--- |FIXED --- Comment #10 from Richard Biener --- Author: rguenth Date: Thu May 23 10:36:55 2013 New Revision: 199242 URL: http://gcc.gnu.org/viewcvs?rev=199242&root=gcc&view=rev Log: 2013-05-23 Richard Biener PR rtl-optimization/57341 * ira.c (validate_equiv_mem_from_store): Use anti_dependence instead of true_dependence. * gcc.dg/torture/pr57341.c: New testcase. Added: branches/gcc-4_8-branch/gcc/testsuite/gcc.dg/torture/pr57341.c Modified: branches/gcc-4_8-branch/gcc/ChangeLog branches/gcc-4_8-branch/gcc/ira.c branches/gcc-4_8-branch/gcc/testsuite/ChangeLog
[Bug target/57341] [4.8/4.9 Regression] wrong code on x86_64-linux at -O3 in 32-bit mode
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57341 --- Comment #9 from Richard Biener --- Author: rguenth Date: Thu May 23 08:37:24 2013 New Revision: 199237 URL: http://gcc.gnu.org/viewcvs?rev=199237&root=gcc&view=rev Log: 2013-05-23 Richard Biener PR rtl-optimization/57341 * ira.c (validate_equiv_mem_from_store): Use anti_dependence instead of true_dependence. * gcc.dg/torture/pr57341.c: New testcase. Added: trunk/gcc/testsuite/gcc.dg/torture/pr57341.c Modified: trunk/gcc/ChangeLog trunk/gcc/ira.c trunk/gcc/testsuite/ChangeLog
[Bug target/57341] [4.8/4.9 Regression] wrong code on x86_64-linux at -O3 in 32-bit mode
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57341
--- Comment #8 from rguenther at suse dot de ---
"jakub at gcc dot gnu.org" wrote:
>http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57341
>
>--- Comment #7 from Jakub Jelinek ---
>(In reply to Michael Matz from comment #5)
>> Nope. Our memory model allows this, the write will dynamically
>change
>> the type of the written memory cell.
>
>But why is that? Just to handle placement new? I'd say it would be
>better to
>make that explicit in the IL.
And all abominations of it. You repeatedly say this and I always respond with
that this is not possible. We have had that at some point and it did not work.
>(In reply to Richard Biener from comment #6)
>> /* { dg-options "-msse" { target { i?86-*-* x86_64-*-* } } } */
>
>looks wrong, that will just crash on pre-SSE CPUs (I know, rare).
>I think better would be:
>
>/* { dg-additional-options "-msse" { target sse2_runtime } } */
>
>(we don't have sse_runtime target, but I guess it doesn't matter if it
>won't be
>tested on pre-SSE2 CPUs, most of them have SSE2 these days).
I'll fix that.
[Bug target/57341] [4.8/4.9 Regression] wrong code on x86_64-linux at -O3 in 32-bit mode
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57341
--- Comment #7 from Jakub Jelinek ---
(In reply to Michael Matz from comment #5)
> Nope. Our memory model allows this, the write will dynamically change
> the type of the written memory cell.
But why is that? Just to handle placement new? I'd say it would be better to
make that explicit in the IL.
(In reply to Richard Biener from comment #6)
> /* { dg-options "-msse" { target { i?86-*-* x86_64-*-* } } } */
looks wrong, that will just crash on pre-SSE CPUs (I know, rare).
I think better would be:
/* { dg-additional-options "-msse" { target sse2_runtime } } */
(we don't have sse_runtime target, but I guess it doesn't matter if it won't be
tested on pre-SSE2 CPUs, most of them have SSE2 these days).
[Bug target/57341] [4.8/4.9 Regression] wrong code on x86_64-linux at -O3 in 32-bit mode
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57341
Richard Biener changed:
What|Removed |Added
Status|NEW |ASSIGNED
Assignee|unassigned at gcc dot gnu.org |rguenth at gcc dot
gnu.org
--- Comment #6 from Richard Biener ---
Testing the patch together with the following
Index: gcc/testsuite/gcc.dg/torture/pr57341.c
===
--- gcc/testsuite/gcc.dg/torture/pr57341.c (revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr57341.c (working copy)
@@ -0,0 +1,22 @@
+/* { dg-do run } */
+/* { dg-options "-msse" { target { i?86-*-* x86_64-*-* } } } */
+
+int a, d;
+int *b = &a, **c;
+int
+main ()
+{
+ int e;
+{
+ int f[4];
+ for (d = 0; d < 4; d++)
+ f[d] = 1;
+ e = f[1];
+}
+ int *g[28] = { };
+ *b = e;
+ c = &g[0];
+ if (a != 1)
+__builtin_abort ();
+ return 0;
+}
[Bug target/57341] [4.8/4.9 Regression] wrong code on x86_64-linux at -O3 in 32-bit mode
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57341 --- Comment #5 from Michael Matz --- (In reply to Jakub Jelinek from comment #4) > (In reply to Richard Biener from comment #3) > > It seems the code really wants to use anti_dependence, not true_dependence. > > We have > > > > ... = equiv_mem; > > dest = ...; > > > > Right. But if equiv_mem overlaps dest and both have non-conflicting alias > sets, > then the program is invalid Nope. Our memory model allows this, the write will dynamically change the type of the written memory cell. > and thus it would be fine to not set > equiv_mem_modified. > > What is the reason why we don't do TBAA checking in anti_dependence etc.? Because we want to allow type changes in the above way. In any case using true_dependence in validate_equiv_mem_from_store definitely is a bug, and anti_dependence is correct.
[Bug target/57341] [4.8/4.9 Regression] wrong code on x86_64-linux at -O3 in 32-bit mode
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57341 Jakub Jelinek changed: What|Removed |Added CC||vmakarov at gcc dot gnu.org --- Comment #4 from Jakub Jelinek --- (In reply to Richard Biener from comment #3) > It seems the code really wants to use anti_dependence, not true_dependence. > We have > > ... = equiv_mem; > dest = ...; > Right. But if equiv_mem overlaps dest and both have non-conflicting alias sets, then the program is invalid and thus it would be fine to not set equiv_mem_modified. What is the reason why we don't do TBAA checking in anti_dependence etc.?
[Bug target/57341] [4.8/4.9 Regression] wrong code on x86_64-linux at -O3 in 32-bit mode
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57341 --- Comment #3 from Richard Biener --- (In reply to Jakub Jelinek from comment #2) > Seems validate_equiv_mem_from_store during update_equiv_regs calls > true_dependence to find out if it is safe to use it as equiv, and > true_dependence is called with > x being > (mem/c:SI (plus:SI (reg/f:SI 20 frame) > (const_int -108 [0xff94])) [3 f+4 S4 A32]) > and mem being > (mem/c:BLK (reg:SI 64) [2 g+0 S112 A128]) > and returns 0 because the alias sets weren't properly adjusted. It seems the code really wants to use anti_dependence, not true_dependence. We have ... = equiv_mem; dest = ...; and the code wants to check whether moving the read before the use, bypassing is possible. Even a write that does not conflict TBAA wise is a barrier for this transform. Index: ira.c === --- ira.c (revision 199199) +++ ira.c (working copy) @@ -2520,7 +2520,7 @@ validate_equiv_mem_from_store (rtx dest, if ((REG_P (dest) && reg_overlap_mentioned_p (dest, equiv_mem)) || (MEM_P (dest) - && true_dependence (dest, VOIDmode, equiv_mem))) + && anti_dependence (equiv_mem, dest))) equiv_mem_modified = 1; }
[Bug target/57341] [4.8/4.9 Regression] wrong code on x86_64-linux at -O3 in 32-bit mode
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57341 Jakub Jelinek changed: What|Removed |Added CC||matz at gcc dot gnu.org --- Comment #2 from Jakub Jelinek --- Seems validate_equiv_mem_from_store during update_equiv_regs calls true_dependence to find out if it is safe to use it as equiv, and true_dependence is called with x being (mem/c:SI (plus:SI (reg/f:SI 20 frame) (const_int -108 [0xff94])) [3 f+4 S4 A32]) and mem being (mem/c:BLK (reg:SI 64) [2 g+0 S112 A128]) and returns 0 because the alias sets weren't properly adjusted.
[Bug target/57341] [4.8/4.9 Regression] wrong code on x86_64-linux at -O3 in 32-bit mode
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57341
Jakub Jelinek changed:
What|Removed |Added
Status|UNCONFIRMED |NEW
Last reconfirmed||2013-05-21
CC||jakub at gcc dot gnu.org
Target Milestone|--- |4.8.1
Summary|wrong code on x86_64-linux |[4.8/4.9 Regression] wrong
|at -O3 in 32-bit mode |code on x86_64-linux at -O3
||in 32-bit mode
Ever confirmed|0 |1
--- Comment #1 from Jakub Jelinek ---
Started with r188667 . -m32 -O3 -msse is needed.
int a, d;
int *b = &a, **c;
int
main ()
{
int e;
{
int f[4];
for (d = 0; d < 4; d++)
f[d] = 1;
e = f[1];
}
int *g[28] = { };
*b = e;
c = &g[0];
if (a != 1)
__builtin_abort ();
return 0;
}
