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 |(a)
will be
 orr %0, %1, #1; it ; 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 | .

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
 wrote:

Hi all,

In this wrong-code PR the pattern for performing
x = x |  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 ; mov%0, #1; it ; orr %0, %1

to emit the RTL equivalent to:
orr %0, %1, #1; it ; 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  

 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  

 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
 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 |(a)
>> will be
>>  orr %0, %1, #1; it ; 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 | .
>
> 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
>>  wrote:
>>>
>>> Hi all,
>>>
>>> In this wrong-code PR the pattern for performing
>>> x = x |  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 ; mov%0, #1; it ; orr %0, %1
>>>
>>> to emit the RTL equivalent to:
>>> orr %0, %1, #1; it ; 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  
>>>
>>>  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  
>>>
>>>  PR target/69403
>>>  * gcc.c-torture/execute/pr69403.c: New test.
>>
>>
>>
>

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

[Kyrill Tkachov]

Hi all,

In this wrong-code PR the pattern for performing
x = x |  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 ; mov%0, #1; it ; orr %0, %1

to emit the RTL equivalent to:
orr %0, %1, #1; it ; 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  

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  

PR target/69403
* gcc.c-torture/execute/pr69403.c: New test.
commit 536a372b7adbb89afa56f61a511ae86e00b7385f
Author: Kyrylo Tkachov 
Date:   Thu Jan 21 10:15:38 2016 +

[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" "=")
 	(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 000..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 |(a)
will be
orr %0, %1, #1; it ; 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
 wrote:
> Hi all,
>
> In this wrong-code PR the pattern for performing
> x = x |  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 ; mov%0, #1; it ; orr %0, %1
>
> to emit the RTL equivalent to:
> orr %0, %1, #1; it ; 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  
>
> 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  
>
> PR target/69403
> * gcc.c-torture/execute/pr69403.c: New test.



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