Message ID | 5AA023DD.8030801@foss.arm.com |
---|---|
State | New |
Headers | show |
Series | [AArch64] PR target/84748: Mark *compare_cstore<mode>_insn as clobbering CC reg | expand |
On Wed, Mar 07, 2018 at 05:39:41PM +0000, Kyrill Tkachov wrote: > Hi all, > > In this wrong-code PR the combine pass ends up moving a CC-using instruction past a *compare_cstore<mode>_insn > insn_and_split. After reload the *compare_cstore<mode>_insn splitter ends up generating a SUBS instruction that > clobbers the condition flags, and things go bad. > > The solution is simple, the *compare_cstore<mode>_insn pattern should specify that it clobbers the CC register > so that combine (or any other pass) does not assume that it can move CC-using patterns across it. > > This patch does that and fixes the testcase. > > The testcase FAILs on GCC 8 only, but the buggy pattern is in GCC 6 onwards, so we should backport this as > a latent bug fix after it's had some time to bake in trunk. > > Bootstrapped and tested on aarch64-none-linux-gnu. > > Ok for trunk and later backports? OK. Thanks, James > 2018-03-07 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR target/84748 > * config/aarch64/aarch64.md (*compare_cstore<mode>_insn): Mark pattern > as clobbering CC_REGNUM. > > 2018-03-07 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR target/84748 > * gcc.c-torture/execute/pr84748.c: New test.
On 08/03/18 13:32, James Greenhalgh wrote: > On Wed, Mar 07, 2018 at 05:39:41PM +0000, Kyrill Tkachov wrote: >> Hi all, >> >> In this wrong-code PR the combine pass ends up moving a CC-using instruction past a *compare_cstore<mode>_insn >> insn_and_split. After reload the *compare_cstore<mode>_insn splitter ends up generating a SUBS instruction that >> clobbers the condition flags, and things go bad. >> >> The solution is simple, the *compare_cstore<mode>_insn pattern should specify that it clobbers the CC register >> so that combine (or any other pass) does not assume that it can move CC-using patterns across it. >> >> This patch does that and fixes the testcase. >> >> The testcase FAILs on GCC 8 only, but the buggy pattern is in GCC 6 onwards, so we should backport this as >> a latent bug fix after it's had some time to bake in trunk. >> >> Bootstrapped and tested on aarch64-none-linux-gnu. >> >> Ok for trunk and later backports? > OK. I've backported the patch to the GCC 6 and 7 branches after bootstrapping and testing there. Thanks, Kyrill > Thanks, > James > >> 2018-03-07 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >> >> PR target/84748 >> * config/aarch64/aarch64.md (*compare_cstore<mode>_insn): Mark pattern >> as clobbering CC_REGNUM. >> >> 2018-03-07 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >> >> PR target/84748 >> * gcc.c-torture/execute/pr84748.c: New test.
commit 9c008e450e1cd95f2905071e6c8a0ff92b856358 Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com> Date: Wed Mar 7 15:05:25 2018 +0000 [AArch64] PR target/84748: Mark *compare_cstore<mode>_insn as clobbering CC reg diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index 69ff5ca..4463a13 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -3242,7 +3242,8 @@ (define_insn "aarch64_cstore<mode>" (define_insn_and_split "*compare_cstore<mode>_insn" [(set (match_operand:GPI 0 "register_operand" "=r") (EQL:GPI (match_operand:GPI 1 "register_operand" "r") - (match_operand:GPI 2 "aarch64_imm24" "n")))] + (match_operand:GPI 2 "aarch64_imm24" "n"))) + (clobber (reg:CC CC_REGNUM))] "!aarch64_move_imm (INTVAL (operands[2]), <MODE>mode) && !aarch64_plus_operand (operands[2], <MODE>mode) && !reload_completed" diff --git a/gcc/testsuite/gcc.c-torture/execute/pr84748.c b/gcc/testsuite/gcc.c-torture/execute/pr84748.c new file mode 100644 index 0000000..9572ab2 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/execute/pr84748.c @@ -0,0 +1,34 @@ +/* { dg-require-effective-target int128 } */ + +typedef unsigned __int128 u128; + +int a, c, d; +u128 b; + +unsigned long long g0, g1; + +void +store (unsigned long long a0, unsigned long long a1) +{ + g0 = a0; + g1 = a1; +} + +void +foo (void) +{ + b += a; + c = d != 84347; + b /= c; + u128 x = b; + store (x >> 0, x >> 64); +} + +int +main (void) +{ + foo (); + if (g0 != 0 || g1 != 0) + __builtin_abort (); + return 0; +}