[PATCH][ARM] PR target/70566 Check that condition register is dead in tst-imm -> lsls-imm Thumb2 peepholes

[PATCH][ARM] PR target/70566 Check that condition register is dead in tst-imm -> lsls-imm Thumb2 peepholes

[Kyrill Tkachov]

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

Hi all,

In this wrong-code PR we have a Thumb2 peephole transforming:
     tst    r3, #2
     bne    .L3
     beq    .L6

into:
     lsls    r3, r3, #30  // LSLS is shorter than TST in Thumb2
     bmi    .L3
     beq    .L6

that is, the branch following the extract+compare has its condition properly changed but the
following branch doesn't get updated to check the opposite condition of MI (PL).
Since the peepholes in thumb2.md only see the compare and a single branch the solution,
suggested by Richard, is to guard those peepholes on the condition that the condition register
is dead after the first branch. This patch does that and with it we no longer perform the transformation
on the testcase. I've checked manually that we still perform the peephole when the condition register
is indeed dead after the sequence.

Bootstrapped and tested on on arm-none-linux-gnueabihf with --with-mode=thumb as this affects only
Thumb2 codegen.

Ok for trunk?

This PR also affects GCC 5 and 4.9 so I'll be testing the patch there as well.

Thanks,
Kyrill


2016-04-07  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR target/70566
     * config/arm/thumb2.md (tst + branch-> lsls + branch
     peephole below *orsi_not_shiftsi_si): Require that condition
     register is dead after the peephole.
     (second peephole after the above): Likewise.

2016-04-07  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR target/70566
     * gcc.c-torture/execute/pr70566.c: New test.

[-- Attachment #2: arm-peepholes.patch --]
[-- Type: text/x-patch, Size: 2024 bytes --]

diff --git a/gcc/config/arm/thumb2.md b/gcc/config/arm/thumb2.md
index 992536593d6c0a8b8fe5a324f32e279c69746157..ab08288413c3e64911e8d7a8199b9809e0282d8e 100644
--- a/gcc/config/arm/thumb2.md
+++ b/gcc/config/arm/thumb2.md
@@ -1550,7 +1550,8 @@ (define_peephole2
 		      (match_operand 5 "" "")
 		      (match_operand 6 "" "")))]
   "TARGET_THUMB2
-   && (INTVAL (operands[2]) >= 0 && INTVAL (operands[2]) < 32)"
+   && (INTVAL (operands[2]) >= 0 && INTVAL (operands[2]) < 32)
+   && peep2_reg_dead_p (2, operands[0])"
   [(parallel [(set (match_dup 0)
 		   (compare:CC_NOOV (ashift:SI (match_dup 1) (match_dup 2))
 				    (const_int 0)))
@@ -1578,7 +1579,8 @@ (define_peephole2
 		      (match_operand 5 "" "")
 		      (match_operand 6 "" "")))]
   "TARGET_THUMB2
-   && (INTVAL (operands[2]) > 0 && INTVAL (operands[2]) < 32)"
+   && (INTVAL (operands[2]) > 0 && INTVAL (operands[2]) < 32)
+   && peep2_reg_dead_p (2, operands[0])"
   [(parallel [(set (match_dup 0)
 		   (compare:CC_NOOV (ashift:SI (match_dup 1) (match_dup 2))
 				    (const_int 0)))
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr70566.c b/gcc/testsuite/gcc.c-torture/execute/pr70566.c
new file mode 100644
index 0000000000000000000000000000000000000000..f47106e70c7d4d7f3623f9505c02445a63332a9d
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr70566.c
@@ -0,0 +1,47 @@
+/* PR target/70566.  */
+
+#define NULL 0
+
+struct mystruct
+{
+  unsigned int f1 : 1;
+  unsigned int f2 : 1;
+  unsigned int f3 : 1;
+};
+
+__attribute__ ((noinline)) void
+myfunc (int a, void *b)
+{
+}
+__attribute__ ((noinline)) int
+myfunc2 (void *a)
+{
+  return 0;
+}
+
+static void
+set_f2 (struct mystruct *user, int f2)
+{
+  if (user->f2 != f2)
+    myfunc (myfunc2 (NULL), NULL);
+  else
+    __builtin_abort ();
+}
+
+__attribute__ ((noinline)) void
+foo (void *data)
+{
+  struct mystruct *user = data;
+  if (!user->f2)
+    set_f2 (user, 1);
+}
+
+int
+main (void)
+{
+  struct mystruct a;
+  a.f1 = 1;
+  a.f2 = 0;
+  foo (&a);
+  return 0;
+}

Re: [PATCH][ARM] PR target/70566 Check that condition register is dead in tst-imm -> lsls-imm Thumb2 peepholes

[Richard Earnshaw (lists)]

On 07/04/16 15:51, Kyrill Tkachov wrote:
> Hi all,
> 
> In this wrong-code PR we have a Thumb2 peephole transforming:
>     tst    r3, #2
>     bne    .L3
>     beq    .L6
> 
> into:
>     lsls    r3, r3, #30  // LSLS is shorter than TST in Thumb2
>     bmi    .L3
>     beq    .L6
> 
> that is, the branch following the extract+compare has its condition
> properly changed but the
> following branch doesn't get updated to check the opposite condition of
> MI (PL).
> Since the peepholes in thumb2.md only see the compare and a single
> branch the solution,
> suggested by Richard, is to guard those peepholes on the condition that
> the condition register
> is dead after the first branch. This patch does that and with it we no
> longer perform the transformation
> on the testcase. I've checked manually that we still perform the
> peephole when the condition register
> is indeed dead after the sequence.
> 
> Bootstrapped and tested on on arm-none-linux-gnueabihf with
> --with-mode=thumb as this affects only
> Thumb2 codegen.
> 
> Ok for trunk?
> 
> This PR also affects GCC 5 and 4.9 so I'll be testing the patch there as
> well.
> 
> Thanks,
> Kyrill
> 
> 
> 2016-04-07  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
>     PR target/70566
>     * config/arm/thumb2.md (tst + branch-> lsls + branch
>     peephole below *orsi_not_shiftsi_si): Require that condition
>     register is dead after the peephole.
>     (second peephole after the above): Likewise.
> 
> 2016-04-07  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
>     PR target/70566
>     * gcc.c-torture/execute/pr70566.c: New test.


OK.

R.

Re: [PATCH][ARM] PR target/70566 Check that condition register is dead in tst-imm -> lsls-imm Thumb2 peepholes

[Kyrill Tkachov]

On 08/04/16 10:30, Richard Earnshaw (lists) wrote:
> On 07/04/16 15:51, Kyrill Tkachov wrote:
>> Hi all,
>>
>> In this wrong-code PR we have a Thumb2 peephole transforming:
>>      tst    r3, #2
>>      bne    .L3
>>      beq    .L6
>>
>> into:
>>      lsls    r3, r3, #30  // LSLS is shorter than TST in Thumb2
>>      bmi    .L3
>>      beq    .L6
>>
>> that is, the branch following the extract+compare has its condition
>> properly changed but the
>> following branch doesn't get updated to check the opposite condition of
>> MI (PL).
>> Since the peepholes in thumb2.md only see the compare and a single
>> branch the solution,
>> suggested by Richard, is to guard those peepholes on the condition that
>> the condition register
>> is dead after the first branch. This patch does that and with it we no
>> longer perform the transformation
>> on the testcase. I've checked manually that we still perform the
>> peephole when the condition register
>> is indeed dead after the sequence.
>>
>> Bootstrapped and tested on on arm-none-linux-gnueabihf with
>> --with-mode=thumb as this affects only
>> Thumb2 codegen.
>>
>> Ok for trunk?
>>
>> This PR also affects GCC 5 and 4.9 so I'll be testing the patch there as
>> well.
>>
>> Thanks,
>> Kyrill
>>
>>
>> 2016-04-07  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
>>      PR target/70566
>>      * config/arm/thumb2.md (tst + branch-> lsls + branch
>>      peephole below *orsi_not_shiftsi_si): Require that condition
>>      register is dead after the peephole.
>>      (second peephole after the above): Likewise.
>>
>> 2016-04-07  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
>>      PR target/70566
>>      * gcc.c-torture/execute/pr70566.c: New test.
>
> OK.

Thanks, the patch has passed bootstrap and test on the GCC 5 and 4.9
branches. Is it ok to backport it there as well?

Thanks,
Kyrill

>
> R.

Re: [PATCH][ARM] PR target/70566 Check that condition register is dead in tst-imm -> lsls-imm Thumb2 peepholes

[Richard Earnshaw (lists)]

On 11/04/16 14:27, Kyrill Tkachov wrote:
> 
> On 08/04/16 10:30, Richard Earnshaw (lists) wrote:
>> On 07/04/16 15:51, Kyrill Tkachov wrote:
>>> Hi all,
>>>
>>> In this wrong-code PR we have a Thumb2 peephole transforming:
>>>      tst    r3, #2
>>>      bne    .L3
>>>      beq    .L6
>>>
>>> into:
>>>      lsls    r3, r3, #30  // LSLS is shorter than TST in Thumb2
>>>      bmi    .L3
>>>      beq    .L6
>>>
>>> that is, the branch following the extract+compare has its condition
>>> properly changed but the
>>> following branch doesn't get updated to check the opposite condition of
>>> MI (PL).
>>> Since the peepholes in thumb2.md only see the compare and a single
>>> branch the solution,
>>> suggested by Richard, is to guard those peepholes on the condition that
>>> the condition register
>>> is dead after the first branch. This patch does that and with it we no
>>> longer perform the transformation
>>> on the testcase. I've checked manually that we still perform the
>>> peephole when the condition register
>>> is indeed dead after the sequence.
>>>
>>> Bootstrapped and tested on on arm-none-linux-gnueabihf with
>>> --with-mode=thumb as this affects only
>>> Thumb2 codegen.
>>>
>>> Ok for trunk?
>>>
>>> This PR also affects GCC 5 and 4.9 so I'll be testing the patch there as
>>> well.
>>>
>>> Thanks,
>>> Kyrill
>>>
>>>
>>> 2016-04-07  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>>
>>>      PR target/70566
>>>      * config/arm/thumb2.md (tst + branch-> lsls + branch
>>>      peephole below *orsi_not_shiftsi_si): Require that condition
>>>      register is dead after the peephole.
>>>      (second peephole after the above): Likewise.
>>>
>>> 2016-04-07  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>>
>>>      PR target/70566
>>>      * gcc.c-torture/execute/pr70566.c: New test.
>>
>> OK.
> 
> Thanks, the patch has passed bootstrap and test on the GCC 5 and 4.9
> branches. Is it ok to backport it there as well?
> 

OK by me.

R.

> Thanks,
> Kyrill
> 
>>
>> R.
>