[PATCH][ARM] Fix PR target/69403: Bug in thumb2_ior_scc_strict_it pattern

[PATCH][ARM] Fix PR target/69403: Bug in thumb2_ior_scc_strict_it pattern

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 1495 bytes --]

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.

[-- Attachment #2: arm-ior-strict-it.patch --]
[-- Type: text/x-patch, Size: 2155 bytes --]

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 ();
+}

Re: [PATCH][ARM] Fix PR target/69403: Bug in thumb2_ior_scc_strict_it pattern

[Han Shen]

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.



-- 
Han Shen |  Software Engineer |  shenhan@google.com |  +1-650-440-3330

Re: [PATCH][ARM] Fix PR target/69403: Bug in thumb2_ior_scc_strict_it pattern

[Kyrill Tkachov]

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.
>
>

Re: [PATCH][ARM] Fix PR target/69403: Bug in thumb2_ior_scc_strict_it pattern

[Ramana Radhakrishnan]

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.
>>
>>
>>
>