[Bug middle-end/70245] [6 Regression] Miscompilation of ICU on i386 with atom tuning starting with r227382
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70245 --- Comment #17 from Jakub Jelinek --- Author: jakub Date: Thu Mar 17 11:53:53 2016 New Revision: 234285 URL: https://gcc.gnu.org/viewcvs?rev=234285=gcc=rev Log: PR target/70245 * rtl.h (replace_rtx): Add ALL_REGS argument. * rtlanal.c (replace_rtx): Likewise. If true, use REGNO equality and assert mode is the same, instead of just rtx pointer equality. * config/i386/i386.md (mov + arithmetics with load peephole): Pass true as ALL_REGS argument to replace_rtx. Modified: trunk/gcc/ChangeLog trunk/gcc/config/i386/i386.md trunk/gcc/rtl.h trunk/gcc/rtlanal.c
[Bug middle-end/70245] [6 Regression] Miscompilation of ICU on i386 with atom tuning starting with r227382
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70245 --- Comment #16 from Jakub Jelinek --- See PR70261.
[Bug middle-end/70245] [6 Regression] Miscompilation of ICU on i386 with atom tuning starting with r227382
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70245 Jakub Jelinek changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #14 from Jakub Jelinek --- Fixed. Note PR70261 tracks regression caused by that change.
[Bug middle-end/70245] [6 Regression] Miscompilation of ICU on i386 with atom tuning starting with r227382
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70245 Jakub Jelinek changed: What|Removed |Added Status|REOPENED|RESOLVED Resolution|--- |FIXED --- Comment #18 from Jakub Jelinek --- Fixed.
[Bug middle-end/70245] [6 Regression] Miscompilation of ICU on i386 with atom tuning starting with r227382
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70245 --- Comment #13 from Jakub Jelinek --- Author: jakub Date: Wed Mar 16 17:52:20 2016 New Revision: 234265 URL: https://gcc.gnu.org/viewcvs?rev=234265=gcc=rev Log: PR target/70245 * rtlanal.c (replace_rtx): For REG, if from is a REG, return to even if only REGNO is equal, and assert mode is the same. * g++.dg/opt/pr70245.C: New test. * g++.dg/opt/pr70245.h: New file. * g++.dg/opt/pr70245-aux.cc: New file. Added: trunk/gcc/testsuite/g++.dg/opt/pr70245-aux.cc trunk/gcc/testsuite/g++.dg/opt/pr70245.C trunk/gcc/testsuite/g++.dg/opt/pr70245.h Modified: trunk/gcc/ChangeLog trunk/gcc/rtlanal.c trunk/gcc/testsuite/ChangeLog
[Bug middle-end/70245] [6 Regression] Miscompilation of ICU on i386 with atom tuning starting with r227382
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70245 Jakub Jelinek changed: What|Removed |Added Status|RESOLVED|REOPENED Resolution|FIXED |--- --- Comment #15 from Jakub Jelinek --- The fix has been reverted, as it caused bootstrap issues on powerpc64* and aarch64 (and perhaps others).
[Bug middle-end/70245] [6 Regression] Miscompilation of ICU on i386 with atom tuning starting with r227382
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70245 --- Comment #12 from Uroš Bizjak --- (In reply to Jakub Jelinek from comment #10) > (In reply to Uroš Bizjak from comment #9) > > Please disable the pattern for gcc-6, and let's fix replace_rtx for gcc-7. > > Disabling the peephole2 entirely would be too aggressive, if we want to > avoid the problems with that, we could as well just guard it with > && !reg_overlap_mentioned_p (operands[0], operands[2]) > or perhaps even allow simple cases like: > && (!reg_overlap_mentioned_p (operands[0], operands[2]) > || rtx_equal_p (operands[0], XEXP (operands[2], 0))) > and in that case of simple REG just replace_equiv_address. > But, the simplify_replace_rtx patch I've attached will do pretty much the > same, it will try to do the peephole2, and punt if the result is not valid > address. > simplify_replace_rtx doesn't simplify anything if there is no change, so > e.g. the > !reg_overlap_mentioned_p (operands[0], operands[2]) case would always work > as it used to. I'm OK with the proposed patch as an interim solution, too - but please add a comment referring to this PR why special handling is needed. > Though, I'm worried about all the other replace_rtx uses in the backends: > config/epiphany/epiphany.md: replace_rtx (operands[2], operands[9], > operands[3]); > config/epiphany/epiphany.md: replace_rtx (operands[2], operands[0], > operands[10]); > config/i386/i386.c: replace_rtx (DF_REF_INSN (ref), reg, scopy); > config/rl78/rl78.c: OP (op) = replace_rtx (OP (op), gen_rtx_REG > (HImode, > SP_REG), HL); > config/rs6000/rs6000.c:real = replace_rtx (real, reg2, rreg); > config/rs6000/rs6000.c:real = replace_rtx (real, reg, > config/sh/sh.md: replace_rtx (operands[4], operands[0], operands[1]); > config/xtensa/xtensa.c: PATTERN (insn) = replace_rtx > (copy_rtx (PATTERN > (insn)), > > The i386.c one is fine, reg is a pseudo, and there should be pointer > equivalency for pseudos I think. What about all the others? It looks to me that the intention of replace_rtx is to replace all occurences of equal RTXes. There is no point of leaving some equals unchanged, as they will probably be clobbered, as is the case with the attached test. But trying to stay on the safe side, the infrastructure change is indeed too risky ATM.
[Bug middle-end/70245] [6 Regression] Miscompilation of ICU on i386 with atom tuning starting with r227382
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70245 --- Comment #11 from Jakub Jelinek --- simplify_replace_rtx is what e.g. postreload uses (together with validate_change to punt if the result isn't valid), which is how I've modeled my patch (there is no insn to validate yet in the peephole2 preparation, which is why it uses memory_operand instead).
[Bug middle-end/70245] [6 Regression] Miscompilation of ICU on i386 with atom tuning starting with r227382
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70245 --- Comment #10 from Jakub Jelinek --- (In reply to Uroš Bizjak from comment #9) > Please disable the pattern for gcc-6, and let's fix replace_rtx for gcc-7. Disabling the peephole2 entirely would be too aggressive, if we want to avoid the problems with that, we could as well just guard it with && !reg_overlap_mentioned_p (operands[0], operands[2]) or perhaps even allow simple cases like: && (!reg_overlap_mentioned_p (operands[0], operands[2]) || rtx_equal_p (operands[0], XEXP (operands[2], 0))) and in that case of simple REG just replace_equiv_address. But, the simplify_replace_rtx patch I've attached will do pretty much the same, it will try to do the peephole2, and punt if the result is not valid address. simplify_replace_rtx doesn't simplify anything if there is no change, so e.g. the !reg_overlap_mentioned_p (operands[0], operands[2]) case would always work as it used to. Though, I'm worried about all the other replace_rtx uses in the backends: config/epiphany/epiphany.md: replace_rtx (operands[2], operands[9], operands[3]); config/epiphany/epiphany.md: replace_rtx (operands[2], operands[0], operands[10]); config/i386/i386.c: replace_rtx (DF_REF_INSN (ref), reg, scopy); config/rl78/rl78.c: OP (op) = replace_rtx (OP (op), gen_rtx_REG (HImode, SP_REG), HL); config/rs6000/rs6000.c:real = replace_rtx (real, reg2, rreg); config/rs6000/rs6000.c:real = replace_rtx (real, reg, config/sh/sh.md: replace_rtx (operands[4], operands[0], operands[1]); config/xtensa/xtensa.c: PATTERN (insn) = replace_rtx (copy_rtx (PATTERN (insn)), The i386.c one is fine, reg is a pseudo, and there should be pointer equivalency for pseudos I think. What about all the others?
[Bug middle-end/70245] [6 Regression] Miscompilation of ICU on i386 with atom tuning starting with r227382
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70245 --- Comment #9 from Uroš Bizjak --- Please disable the pattern for gcc-6, and let's fix replace_rtx for gcc-7.
[Bug middle-end/70245] [6 Regression] Miscompilation of ICU on i386 with atom tuning starting with r227382
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70245 --- Comment #8 from Jakub Jelinek --- Though, looking at other spots that are using replace_rtx, e.g. sh and epiphany also use it in define_peephole2, thus post reload, and there are tons of other calls that probably want to use rtx_equal_p instead of x == from.
[Bug middle-end/70245] [6 Regression] Miscompilation of ICU on i386 with atom tuning starting with r227382
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70245
--- Comment #7 from Jakub Jelinek ---
Created attachment 37988
--> https://gcc.gnu.org/bugzilla/attachment.cgi?id=37988=edit
gcc6-pr70245.patch
So, I think the options we have are:
1) the attached patch, use simplify_replace_rtx which uses rtx_equal_p and
should most of the time DTRT, though theoretically it is possible it might try
to simplify a valid memory address into invalid one (which is why I've added
there the conditional FAIL just in case)
2) use the rtl-iter.h iterators and do something like:
subrtx_ptr_iterator::array_type array;
operands[4] = operands[2];
FOR_EACH_SUBRTX_PTR (iter, array, loc, NONCONST)
{
rtx *loc = *iter;
rtx x = *loc;
if (rtx_equal_p (x, operands[0]))
*loc = operands[1];
}
manually, the problem is that the insn-*.c generated files don't include the
rtl-iter.h header, so this would need to be outlined into some helper function
in i386.c
3) change replace_rtx, so that it doesn't check for pointer equality, but does
rtx_equal_p.
I find the 3) change just too risky, at least at this point, as it is unclear
if the current other callers of replace_rtx aren't relying on it replacing just
exactly what they asked it to.
[Bug middle-end/70245] [6 Regression] Miscompilation of ICU on i386 with atom tuning starting with r227382
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70245 --- Comment #6 from Jakub Jelinek --- Ah, you're right, sorry for blindly CCing you after bisecting to a RA related commit. The peephole would be fine if replace_rtx worked; unfortunately it does pointer comparison (x == from) rather than rtx_equal_p.
[Bug middle-end/70245] [6 Regression] Miscompilation of ICU on i386 with atom tuning starting with r227382
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70245
--- Comment #5 from Vladimir Makarov ---
(In reply to Vladimir Makarov from comment #4)
>
> I see in peephole a wrong transformation:
>
> 190: cx:SI=dx:SI
> REG_DEAD dx:SI
>95: {cx:SI=cx:SI+[cx:SI];clobber flags:CC;}
>
> into
>
> 242: cx:SI=[cx:SI]
> 243: {cx:SI=cx:SI+dx:SI;clobber flags:CC;}
The pattern in question:
(define_peephole2
[(set (match_operand:SI 0 "register_operand")
(match_operand:SI 1 "register_operand"))
(parallel [(set (match_dup 0)
(match_operator:SI 3 "commutative_operator"
[(match_dup 0)
(match_operand:SI 2 "memory_operand")]))
(clobber (reg:CC FLAGS_REG))])]
"REGNO (operands[0]) != REGNO (operands[1])
&& GENERAL_REGNO_P (REGNO (operands[0]))
&& GENERAL_REGNO_P (REGNO (operands[1]))"
[(set (match_dup 0) (match_dup 4))
(parallel [(set (match_dup 0)
(match_op_dup 3 [(match_dup 0) (match_dup 1)]))
(clobber (reg:CC FLAGS_REG))])]
"operands[4] = replace_rtx (operands[2], operands[0], operands[1]);")
[Bug middle-end/70245] [6 Regression] Miscompilation of ICU on i386 with atom tuning starting with r227382
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70245
--- Comment #4 from Vladimir Makarov ---
(In reply to Jakub Jelinek from comment #3)
> The difference between r227381 and r227382 on the testcase is:
> --- r227382-1.s1 2016-03-15 20:34:52.699640513 +0100
> +++ r227382-1.s2 2016-03-15 20:35:05.278470114 +0100
> @@ -116,7 +116,7 @@ _ZL3barP1BPi:
> movl24(%esp), %edi
> movl%eax, 24(%esp)
> movl%edi, 12(%esp)
> - movl(%edx), %ecx
> + movl(%edi), %ecx
> movl%eax, (%esp)
> addl%edx, %ecx
> movl%ebp, 4(%esp)
> which is the load of g->d in d's arguments, %edi at this point isn't equal
> to g, while %edx is (you can see it a few insns later, g and f are not
> modified after assignment and aren't addressable, thus they have the same
> value, and so it is actually f + f->d, in the addition it is either correct
> %edx + (%edx), or incorrect %edx + (%edi) where %edi happens to point to 0
> instead of the right 4.
I see in peephole a wrong transformation:
190: cx:SI=dx:SI
REG_DEAD dx:SI
95: {cx:SI=cx:SI+[cx:SI];clobber flags:CC;}
into
242: cx:SI=[cx:SI]
243: {cx:SI=cx:SI+dx:SI;clobber flags:CC;}
[Bug middle-end/70245] [6 Regression] Miscompilation of ICU on i386 with atom tuning starting with r227382
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70245 Jakub Jelinek changed: What|Removed |Added Keywords||wrong-code Status|UNCONFIRMED |NEW Last reconfirmed||2016-03-15 Ever confirmed|0 |1 --- Comment #3 from Jakub Jelinek --- The difference between r227381 and r227382 on the testcase is: --- r227382-1.s12016-03-15 20:34:52.699640513 +0100 +++ r227382-1.s22016-03-15 20:35:05.278470114 +0100 @@ -116,7 +116,7 @@ _ZL3barP1BPi: movl24(%esp), %edi movl%eax, 24(%esp) movl%edi, 12(%esp) - movl(%edx), %ecx + movl(%edi), %ecx movl%eax, (%esp) addl%edx, %ecx movl%ebp, 4(%esp) which is the load of g->d in d's arguments, %edi at this point isn't equal to g, while %edx is (you can see it a few insns later, g and f are not modified after assignment and aren't addressable, thus they have the same value, and so it is actually f + f->d, in the addition it is either correct %edx + (%edx), or incorrect %edx + (%edi) where %edi happens to point to 0 instead of the right 4.
[Bug middle-end/70245] [6 Regression] Miscompilation of ICU on i386 with atom tuning starting with r227382
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70245 --- Comment #2 from Jakub Jelinek --- Created attachment 37979 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=37979=edit r227382-2.C
[Bug middle-end/70245] [6 Regression] Miscompilation of ICU on i386 with atom tuning starting with r227382
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70245 --- Comment #1 from Jakub Jelinek --- Created attachment 37978 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=37978=edit r227382-1.C
[Bug middle-end/70245] [6 Regression] Miscompilation of ICU on i386 with atom tuning starting with r227382
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70245 Jakub Jelinek changed: What|Removed |Added Priority|P3 |P1 CC||vmakarov at gcc dot gnu.org Target Milestone|--- |6.0
