| Message ID | yxj5hb1gzcli.fsf@build3-lucid-cs.sje.mentorg.com |
|---|---|
| State | New |
| Headers | show |
On 04/12/11 13:32, Kazu_Hirata@mentor.com wrote: > Hi, > > Attached is a patch to fix miscompilation in > arm.md:*minmax_arithsi. > > The following testcase, reduced from efgcvt_r.c:fcvt_r in glibc, gets > miscompiled: > > extern void abort (void); > > int __attribute__((noinline)) > foo (int a, int b) > { > int max = (b > 0) ? b : 0; > return max - a; > } > > int > main (void) > { > if (foo (3, -1) != -3) > abort (); > return 0; > } > > arm-none-eabi-gcc -O1 generates: > > foo: > @ Function supports interworking. > @ args = 0, pretend = 0, frame = 0 > @ frame_needed = 0, uses_anonymous_args = 0 > @ link register save eliminated. > cmp r1, #0 > rsbge r0, r0, r1 > bx lr > > This would be equivalent to: > > return b >= 0 ? b - a : a; > > which is different from: > > return b >= 0 ? b - a : -a; > > That is, in assembly code, we should have an "else" clause like so: > > cmp r1, #0 > rsbge r0, r0, r1 <- then clause > rsblt r0, r0, #0 <- else clause > bx lr > > The problem comes from the fact that arm.md:*minmax_arithsi does not > add the "else" clause even though MINUS is not commutative. > > The patch fixes the problem by always requiring the "else" clause in > the MINUS case. > > Tested by running gcc testsuite on various ARM subarchitectures. OK > to apply? > > Kazu Hirata > > gcc/ > 2011-12-04 Kazu Hirata <kazu@codesourcery.com> > > PR target/51408 > * config/arm/arm.md (*minmax_arithsi): Always require the else > clause in the MINUS case. > > gcc/testsuite/ > 2011-12-04 Kazu Hirata <kazu@codesourcery.com> > > PR target/51408 > * gcc.dg/pr51408.c: New. > OK. R.
On 05/12/11 10:42, Richard Earnshaw wrote: > On 04/12/11 13:32, Kazu_Hirata@mentor.com wrote: >> Hi, >> >> Attached is a patch to fix miscompilation in >> arm.md:*minmax_arithsi. >> >> The following testcase, reduced from efgcvt_r.c:fcvt_r in glibc, gets >> miscompiled: >> >> extern void abort (void); >> >> int __attribute__((noinline)) >> foo (int a, int b) >> { >> int max = (b > 0) ? b : 0; >> return max - a; >> } >> >> int >> main (void) >> { >> if (foo (3, -1) != -3) >> abort (); >> return 0; >> } >> >> arm-none-eabi-gcc -O1 generates: >> >> foo: >> @ Function supports interworking. >> @ args = 0, pretend = 0, frame = 0 >> @ frame_needed = 0, uses_anonymous_args = 0 >> @ link register save eliminated. >> cmp r1, #0 >> rsbge r0, r0, r1 >> bx lr >> >> This would be equivalent to: >> >> return b >= 0 ? b - a : a; >> >> which is different from: >> >> return b >= 0 ? b - a : -a; >> >> That is, in assembly code, we should have an "else" clause like so: >> >> cmp r1, #0 >> rsbge r0, r0, r1 <- then clause >> rsblt r0, r0, #0 <- else clause >> bx lr >> >> The problem comes from the fact that arm.md:*minmax_arithsi does not >> add the "else" clause even though MINUS is not commutative. >> >> The patch fixes the problem by always requiring the "else" clause in >> the MINUS case. >> >> Tested by running gcc testsuite on various ARM subarchitectures. OK >> to apply? >> >> Kazu Hirata >> >> gcc/ >> 2011-12-04 Kazu Hirata <kazu@codesourcery.com> >> >> PR target/51408 >> * config/arm/arm.md (*minmax_arithsi): Always require the else >> clause in the MINUS case. >> >> gcc/testsuite/ >> 2011-12-04 Kazu Hirata <kazu@codesourcery.com> >> >> PR target/51408 >> * gcc.dg/pr51408.c: New. >> > > OK. > > R. BTW, I would expect this to also exist in all the release branches. Could you back-port it where needed please. TIA, R.
Hi Richard, > BTW, I would expect this to also exist in all the release branches. Could you back-port it where > needed please. I just backported to 4.4, 4.5, and 4.6 branches. Kazu Hirata
Index: gcc/config/arm/arm.md =================================================================== --- gcc/config/arm/arm.md (revision 181985) +++ gcc/config/arm/arm.md (working copy) @@ -3413,7 +3413,7 @@ bool need_else; if (which_alternative != 0 || operands[3] != const0_rtx - || (code != PLUS && code != MINUS && code != IOR && code != XOR)) + || (code != PLUS && code != IOR && code != XOR)) need_else = true; else need_else = false; Index: gcc/testsuite/gcc.dg/pr51408.c =================================================================== --- gcc/testsuite/gcc.dg/pr51408.c (revision 0) +++ gcc/testsuite/gcc.dg/pr51408.c (revision 0) @@ -0,0 +1,22 @@ +/* This testcase used to fail because of a bug in + arm.md:*minmax_arithsi. */ + +/* { dg-do run } */ +/* { dg-options "-O1" } */ + +extern void abort (void); + +int __attribute__((noinline)) +foo (int a, int b) +{ + int max = (b > 0) ? b : 0; + return max - a; +} + +int +main (void) +{ + if (foo (3, -1) != -3) + abort (); + return 0; +}
Hi, Attached is a patch to fix miscompilation in arm.md:*minmax_arithsi. The following testcase, reduced from efgcvt_r.c:fcvt_r in glibc, gets miscompiled: extern void abort (void); int __attribute__((noinline)) foo (int a, int b) { int max = (b > 0) ? b : 0; return max - a; } int main (void) { if (foo (3, -1) != -3) abort (); return 0; } arm-none-eabi-gcc -O1 generates: foo: @ Function supports interworking. @ args = 0, pretend = 0, frame = 0 @ frame_needed = 0, uses_anonymous_args = 0 @ link register save eliminated. cmp r1, #0 rsbge r0, r0, r1 bx lr This would be equivalent to: return b >= 0 ? b - a : a; which is different from: return b >= 0 ? b - a : -a; That is, in assembly code, we should have an "else" clause like so: cmp r1, #0 rsbge r0, r0, r1 <- then clause rsblt r0, r0, #0 <- else clause bx lr The problem comes from the fact that arm.md:*minmax_arithsi does not add the "else" clause even though MINUS is not commutative. The patch fixes the problem by always requiring the "else" clause in the MINUS case. Tested by running gcc testsuite on various ARM subarchitectures. OK to apply? Kazu Hirata gcc/ 2011-12-04 Kazu Hirata <kazu@codesourcery.com> PR target/51408 * config/arm/arm.md (*minmax_arithsi): Always require the else clause in the MINUS case. gcc/testsuite/ 2011-12-04 Kazu Hirata <kazu@codesourcery.com> PR target/51408 * gcc.dg/pr51408.c: New.