| Message ID | 56A11A98.1020503@foss.arm.com |
|---|---|
| State | New |
| Headers | show |
Hi Kyrill, the patched gcc generates correct asm for me for the test
case. (I'll then build the whole system to see if other it-block
related bugs are gone too.)
One short question, the newly generated RTL for
x = x | <cond> (a)
will be
orr %0, %1, #1; it <cond>; mov%D2\\t%0, %1 (b)
The cond in (a) should be the reverse of cond in(b), right?
Thanks for your quick fix.
Han
On Thu, Jan 21, 2016 at 9:51 AM, Kyrill Tkachov
<kyrylo.tkachov@foss.arm.com> wrote:
> Hi all,
>
> In this wrong-code PR the pattern for performing
> x = x | <cond> for -mrestrict-it is buggy and ends up writing 1 to the
> result register rather than orring it.
>
> The offending pattern is *thumb2_ior_scc_strict_it.
> My proposed solution is to rewrite it as a splitter, remove the
> alternative for the case where operands[1] and 0 are the same reg
> that emits the bogus:
> it <cond>; mov<cond>%0, #1; it <cond>; orr<cond> %0, %1
>
> to emit the RTL equivalent to:
> orr %0, %1, #1; it <cond>; mov%D2\\t%0, %1
> while marking operand 0 as an earlyclobber operand so that it doesn't
> get assigned the same register as operand 1.
>
> This way we avoid the wrong-code, make the sequence better (by eliminating
> the move of #1 into a register
> and relaxing the constraints from 'l' to 'r' since only the register move
> has to be conditional).
> and still stay within the rules for arm_restrict_it.
>
> Bootstrapped and tested on arm-none-linux-gnueabihf configured
> --with-arch=armv8-a and --with-mode=thumb.
>
> Ok for trunk, GCC 5 and 4.9?
>
> Han, can you please try this out to see if it solves the problem on your end
> as well?
>
> Thanks,
> Kyrill
>
> 2016-01-21 Kyrylo Tkachov <kyrylo.tkachov@arm.com>
>
> PR target/69403
> * config/arm/thumb2.md (*thumb2_ior_scc_strict_it): Convert to
> define_insn_and_split. Ensure operands[1] and operands[0] do not
> get assigned the same register.
>
> 2016-01-21 Kyrylo Tkachov <kyrylo.tkachov@arm.com>
>
> PR target/69403
> * gcc.c-torture/execute/pr69403.c: New test.
Hi Han, On 21/01/16 22:57, Han Shen wrote: > Hi Kyrill, the patched gcc generates correct asm for me for the test > case. (I'll then build the whole system to see if other it-block > related bugs are gone too.) > > One short question, the newly generated RTL for > x = x | <cond> (a) > will be > orr %0, %1, #1; it <cond>; mov%D2\\t%0, %1 (b) > > The cond in (a) should be the reverse of cond in(b), right? yes, the first C-like expression is just some pseudocode. I guess it would be better written as: x = x | <comparison result>. Thanks for trying it out. I bootstrapped and tested this patch on trunk and GCC 5. I'll be doing the same on the 4.9 branch. Kyrill > Thanks for your quick fix. > > Han > > On Thu, Jan 21, 2016 at 9:51 AM, Kyrill Tkachov > <kyrylo.tkachov@foss.arm.com> wrote: >> Hi all, >> >> In this wrong-code PR the pattern for performing >> x = x | <cond> for -mrestrict-it is buggy and ends up writing 1 to the >> result register rather than orring it. >> >> The offending pattern is *thumb2_ior_scc_strict_it. >> My proposed solution is to rewrite it as a splitter, remove the >> alternative for the case where operands[1] and 0 are the same reg >> that emits the bogus: >> it <cond>; mov<cond>%0, #1; it <cond>; orr<cond> %0, %1 >> >> to emit the RTL equivalent to: >> orr %0, %1, #1; it <cond>; mov%D2\\t%0, %1 >> while marking operand 0 as an earlyclobber operand so that it doesn't >> get assigned the same register as operand 1. >> >> This way we avoid the wrong-code, make the sequence better (by eliminating >> the move of #1 into a register >> and relaxing the constraints from 'l' to 'r' since only the register move >> has to be conditional). >> and still stay within the rules for arm_restrict_it. >> >> Bootstrapped and tested on arm-none-linux-gnueabihf configured >> --with-arch=armv8-a and --with-mode=thumb. >> >> Ok for trunk, GCC 5 and 4.9? >> >> Han, can you please try this out to see if it solves the problem on your end >> as well? >> >> Thanks, >> Kyrill >> >> 2016-01-21 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >> >> PR target/69403 >> * config/arm/thumb2.md (*thumb2_ior_scc_strict_it): Convert to >> define_insn_and_split. Ensure operands[1] and operands[0] do not >> get assigned the same register. >> >> 2016-01-21 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >> >> PR target/69403 >> * gcc.c-torture/execute/pr69403.c: New test. > >
On Fri, Jan 22, 2016 at 9:32 AM, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote: > Hi Han, > > On 21/01/16 22:57, Han Shen wrote: >> >> Hi Kyrill, the patched gcc generates correct asm for me for the test >> case. (I'll then build the whole system to see if other it-block >> related bugs are gone too.) >> >> One short question, the newly generated RTL for >> x = x | <cond> (a) >> will be >> orr %0, %1, #1; it <cond>; mov%D2\\t%0, %1 (b) >> >> The cond in (a) should be the reverse of cond in(b), right? > > > yes, the first C-like expression is just some pseudocode. > I guess it would be better written as: > x = x | <comparison result>. > > Thanks for trying it out. > I bootstrapped and tested this patch on trunk and GCC 5. > I'll be doing the same on the 4.9 branch. Ok everywhere. While you are here could you also audit the other patterns that we changed for restrict_it to check that this thinko isn't present anywhere else, please ? Ramana > > Kyrill > > >> Thanks for your quick fix. >> >> Han >> >> On Thu, Jan 21, 2016 at 9:51 AM, Kyrill Tkachov >> <kyrylo.tkachov@foss.arm.com> wrote: >>> >>> Hi all, >>> >>> In this wrong-code PR the pattern for performing >>> x = x | <cond> for -mrestrict-it is buggy and ends up writing 1 to the >>> result register rather than orring it. >>> >>> The offending pattern is *thumb2_ior_scc_strict_it. >>> My proposed solution is to rewrite it as a splitter, remove the >>> alternative for the case where operands[1] and 0 are the same reg >>> that emits the bogus: >>> it <cond>; mov<cond>%0, #1; it <cond>; orr<cond> %0, %1 >>> >>> to emit the RTL equivalent to: >>> orr %0, %1, #1; it <cond>; mov%D2\\t%0, %1 >>> while marking operand 0 as an earlyclobber operand so that it doesn't >>> get assigned the same register as operand 1. >>> >>> This way we avoid the wrong-code, make the sequence better (by >>> eliminating >>> the move of #1 into a register >>> and relaxing the constraints from 'l' to 'r' since only the register move >>> has to be conditional). >>> and still stay within the rules for arm_restrict_it. >>> >>> Bootstrapped and tested on arm-none-linux-gnueabihf configured >>> --with-arch=armv8-a and --with-mode=thumb. >>> >>> Ok for trunk, GCC 5 and 4.9? >>> >>> Han, can you please try this out to see if it solves the problem on your >>> end >>> as well? >>> >>> Thanks, >>> Kyrill >>> >>> 2016-01-21 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >>> >>> PR target/69403 >>> * config/arm/thumb2.md (*thumb2_ior_scc_strict_it): Convert to >>> define_insn_and_split. Ensure operands[1] and operands[0] do not >>> get assigned the same register. >>> >>> 2016-01-21 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >>> >>> PR target/69403 >>> * gcc.c-torture/execute/pr69403.c: New test. >> >> >> >
commit 536a372b7adbb89afa56f61a511ae86e00b7385f
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date: Thu Jan 21 10:15:38 2016 +0000
[ARM] Fix PR target/69403: Bug in thumb2_ior_scc_strict_it pattern
diff --git a/gcc/config/arm/thumb2.md b/gcc/config/arm/thumb2.md
index 7368d06..9925365 100644
--- a/gcc/config/arm/thumb2.md
+++ b/gcc/config/arm/thumb2.md
@@ -663,15 +663,27 @@ (define_insn_and_split "*thumb2_ior_scc"
(set_attr "type" "multiple")]
)
-(define_insn "*thumb2_ior_scc_strict_it"
- [(set (match_operand:SI 0 "s_register_operand" "=l,l")
+(define_insn_and_split "*thumb2_ior_scc_strict_it"
+ [(set (match_operand:SI 0 "s_register_operand" "=&r")
(ior:SI (match_operator:SI 2 "arm_comparison_operator"
[(match_operand 3 "cc_register" "") (const_int 0)])
- (match_operand:SI 1 "s_register_operand" "0,?l")))]
+ (match_operand:SI 1 "s_register_operand" "r")))]
"TARGET_THUMB2 && arm_restrict_it"
- "@
- it\\t%d2\;mov%d2\\t%0, #1\;it\\t%d2\;orr%d2\\t%0, %1
- mov\\t%0, #1\;orr\\t%0, %1\;it\\t%D2\;mov%D2\\t%0, %1"
+ "#" ; orr\\t%0, %1, #1\;it\\t%D2\;mov%D2\\t%0, %1
+ "&& reload_completed"
+ [(set (match_dup 0) (ior:SI (match_dup 1) (const_int 1)))
+ (cond_exec (match_dup 4)
+ (set (match_dup 0) (match_dup 1)))]
+ {
+ machine_mode mode = GET_MODE (operands[3]);
+ rtx_code rc = GET_CODE (operands[2]);
+
+ if (mode == CCFPmode || mode == CCFPEmode)
+ rc = reverse_condition_maybe_unordered (rc);
+ else
+ rc = reverse_condition (rc);
+ operands[4] = gen_rtx_fmt_ee (rc, VOIDmode, operands[3], const0_rtx);
+ }
[(set_attr "conds" "use")
(set_attr "length" "8")
(set_attr "type" "multiple")]
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr69403.c b/gcc/testsuite/gcc.c-torture/execute/pr69403.c
new file mode 100644
index 0000000..097d366
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr69403.c
@@ -0,0 +1,20 @@
+/* PR target/69403. */
+
+int a, b, c;
+
+__attribute__ ((__noinline__)) int
+fn1 ()
+{
+ if ((b | (a != (a & c))) == 1)
+ __builtin_abort ();
+ return 0;
+}
+
+int
+main (void)
+{
+ a = 5;
+ c = 1;
+ b = 6;
+ return fn1 ();
+}
Hi all, In this wrong-code PR the pattern for performing x = x | <cond> for -mrestrict-it is buggy and ends up writing 1 to the result register rather than orring it. The offending pattern is *thumb2_ior_scc_strict_it. My proposed solution is to rewrite it as a splitter, remove the alternative for the case where operands[1] and 0 are the same reg that emits the bogus: it <cond>; mov<cond>%0, #1; it <cond>; orr<cond> %0, %1 to emit the RTL equivalent to: orr %0, %1, #1; it <cond>; mov%D2\\t%0, %1 while marking operand 0 as an earlyclobber operand so that it doesn't get assigned the same register as operand 1. This way we avoid the wrong-code, make the sequence better (by eliminating the move of #1 into a register and relaxing the constraints from 'l' to 'r' since only the register move has to be conditional). and still stay within the rules for arm_restrict_it. Bootstrapped and tested on arm-none-linux-gnueabihf configured --with-arch=armv8-a and --with-mode=thumb. Ok for trunk, GCC 5 and 4.9? Han, can you please try this out to see if it solves the problem on your end as well? Thanks, Kyrill 2016-01-21 Kyrylo Tkachov <kyrylo.tkachov@arm.com> PR target/69403 * config/arm/thumb2.md (*thumb2_ior_scc_strict_it): Convert to define_insn_and_split. Ensure operands[1] and operands[0] do not get assigned the same register. 2016-01-21 Kyrylo Tkachov <kyrylo.tkachov@arm.com> PR target/69403 * gcc.c-torture/execute/pr69403.c: New test.