[PATCH][ARM] PR target/70008

[PATCH][ARM] PR target/70008

[Michael Collison]

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

This patches address PR 70008, where a reverse subtract with carry 
instruction can be generated in thumb2 mode. It was tested with no 
regressions in arm and thumb modes on the following targets:

arm-none-linux-gnueabi
arm-none-linux-gnuabihf
armeb-none-linux-gnuabihf
arm-none-eabi

Okay for trunk?

2016-02-28  Michael Collison  <michael.collison@linaro.org>

     PR target/70008
     * config/arm/arm.md (*subsi3_carryin): Only match pattern if
     TARGET_ARM due to 'rsc' instruction alternative.
     * config/arm/thumb2.md (*thumb2_subsi3_carryin): New pattern.


-- 
Michael Collison
Linaro Toolchain Working Group
michael.collison@linaro.org


[-- Attachment #2: bugzilla-70007-upstream-v1.patch --]
[-- Type: text/x-patch, Size: 1367 bytes --]

diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index e67239d..a008207 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -870,7 +870,7 @@
         (minus:SI (minus:SI (match_operand:SI 1 "reg_or_int_operand" "r,I")
                             (match_operand:SI 2 "s_register_operand" "r,r"))
                   (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))]
-  "TARGET_32BIT"
+  "TARGET_ARM"
   "@
    sbc%?\\t%0, %1, %2
    rsc%?\\t%0, %2, %1"
diff --git a/gcc/config/arm/thumb2.md b/gcc/config/arm/thumb2.md
index 9925365..79305c5 100644
--- a/gcc/config/arm/thumb2.md
+++ b/gcc/config/arm/thumb2.md
@@ -848,6 +848,20 @@
    (set_attr "type" "multiple")]
 )
 
+(define_insn "*thumb2_subsi3_carryin"
+  [(set (match_operand:SI 0 "s_register_operand" "=r")
+        (minus:SI (minus:SI (match_operand:SI 1 "s_register_operand" "r")
+                            (match_operand:SI 2 "s_register_operand" "r"))
+                  (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))]
+  "TARGET_THUMB2"
+  "@
+   sbc%?\\t%0, %1, %2"
+  [(set_attr "conds" "use")
+   (set_attr "predicable" "yes")
+   (set_attr "predicable_short_it" "no")
+   (set_attr "type" "adc_reg")]
+)
+
 (define_insn "*thumb2_cond_sub"
   [(set (match_operand:SI 0 "s_register_operand" "=Ts,Ts")
         (minus:SI (match_operand:SI 1 "s_register_operand" "0,?Ts")
-- 
1.9.1

Re: [PATCH][ARM] PR target/70008

[Kyrill Tkachov]

Hi Michael,

On 29/02/16 04:47, Michael Collison wrote:
> This patches address PR 70008, where a reverse subtract with carry instruction can be generated in thumb2 mode. It was tested with no regressions in arm and thumb modes on the following targets:
>
> arm-none-linux-gnueabi
> arm-none-linux-gnuabihf
> armeb-none-linux-gnuabihf
> arm-none-eabi
>
> Okay for trunk?
>
> 2016-02-28  Michael Collison  <michael.collison@linaro.org>
>
>     PR target/70008
>     * config/arm/arm.md (*subsi3_carryin): Only match pattern if
>     TARGET_ARM due to 'rsc' instruction alternative.
>     * config/arm/thumb2.md (*thumb2_subsi3_carryin): New pattern.
>
>

The *subsi3_carrying pattern has the arch attribute:
    (set_attr "arch" "*,a")

That means that the second alternative that generates the RSC instruction is only enabled
for ARM mode. Do you have a testcase where this doesn't happen and this pattern generates
the second alternative for Thumb2?

Thanks,
Kyrill

Re: [PATCH][ARM] PR target/70008

[Michael Collison]

On 2/29/2016 4:06 AM, Kyrill Tkachov wrote:
> Hi Michael,
>
> On 29/02/16 04:47, Michael Collison wrote:
>> This patches address PR 70008, where a reverse subtract with carry 
>> instruction can be generated in thumb2 mode. It was tested with no 
>> regressions in arm and thumb modes on the following targets:
>>
>> arm-none-linux-gnueabi
>> arm-none-linux-gnuabihf
>> armeb-none-linux-gnuabihf
>> arm-none-eabi
>>
>> Okay for trunk?
>>
>> 2016-02-28  Michael Collison <michael.collison@linaro.org>
>>
>>     PR target/70008
>>     * config/arm/arm.md (*subsi3_carryin): Only match pattern if
>>     TARGET_ARM due to 'rsc' instruction alternative.
>>     * config/arm/thumb2.md (*thumb2_subsi3_carryin): New pattern.
>>
>>
>
> The *subsi3_carrying pattern has the arch attribute:
>    (set_attr "arch" "*,a")
>
> That means that the second alternative that generates the RSC 
> instruction is only enabled
> for ARM mode. Do you have a testcase where this doesn't happen and 
> this pattern generates
> the second alternative for Thumb2?

No I don't have a test case; i noticed the pattern when working on the 
overflow project. I did not realize
that an attribute could affect the matching of an alternative. I will 
close the bug.

>
>
> Thanks,
> Kyrill

Re: [PATCH][ARM] PR target/70008

[Richard Earnshaw (lists)]

On 29/02/16 11:21, Michael Collison wrote:
> 
> 
> On 2/29/2016 4:06 AM, Kyrill Tkachov wrote:
>> Hi Michael,
>>
>> On 29/02/16 04:47, Michael Collison wrote:
>>> This patches address PR 70008, where a reverse subtract with carry
>>> instruction can be generated in thumb2 mode. It was tested with no
>>> regressions in arm and thumb modes on the following targets:
>>>
>>> arm-none-linux-gnueabi
>>> arm-none-linux-gnuabihf
>>> armeb-none-linux-gnuabihf
>>> arm-none-eabi
>>>
>>> Okay for trunk?
>>>
>>> 2016-02-28  Michael Collison <michael.collison@linaro.org>
>>>
>>>     PR target/70008
>>>     * config/arm/arm.md (*subsi3_carryin): Only match pattern if
>>>     TARGET_ARM due to 'rsc' instruction alternative.
>>>     * config/arm/thumb2.md (*thumb2_subsi3_carryin): New pattern.
>>>
>>>
>>
>> The *subsi3_carrying pattern has the arch attribute:
>>    (set_attr "arch" "*,a")
>>
>> That means that the second alternative that generates the RSC
>> instruction is only enabled
>> for ARM mode. Do you have a testcase where this doesn't happen and
>> this pattern generates
>> the second alternative for Thumb2?
> 
> No I don't have a test case; i noticed the pattern when working on the
> overflow project. I did not realize
> that an attribute could affect the matching of an alternative. I will
> close the bug.
> 
>>
>>
>> Thanks,
>> Kyrill
> 

This is all true, but there is a potential performance issue with this
pattern though, that could lead to sub-optimal code.

The predicate accepts reg-or-int, but in ARM state only simple
'const-ok-for-arm' immediates are permitted by the predicates, and in
thumb code, no immediates are permitted at all.  This could potentially
result in sub-optimal code due to late splitting of the pattern.  It
would be better if the predicate understood these limitations and
restricted immediates accordingly.

R.

Re: [PATCH][ARM] PR target/70008

[Michael Collison]

Hi Richard,

I think we could incorporate your feedback by changing the predicate on 
operand 1 to "arm_rhs_operand" which allows "s_register_operand" or 
"arm_immediate_operand". Everything else in my patch would stay the same 
including splitting the thumb2 pattern out into it's own insn. I'm 
testing this change now. Let me know if this direction is okay with you.

On 02/29/2016 08:29 AM, Richard Earnshaw (lists) wrote:
> On 29/02/16 11:21, Michael Collison wrote:
>>
>> On 2/29/2016 4:06 AM, Kyrill Tkachov wrote:
>>> Hi Michael,
>>>
>>> On 29/02/16 04:47, Michael Collison wrote:
>>>> This patches address PR 70008, where a reverse subtract with carry
>>>> instruction can be generated in thumb2 mode. It was tested with no
>>>> regressions in arm and thumb modes on the following targets:
>>>>
>>>> arm-none-linux-gnueabi
>>>> arm-none-linux-gnuabihf
>>>> armeb-none-linux-gnuabihf
>>>> arm-none-eabi
>>>>
>>>> Okay for trunk?
>>>>
>>>> 2016-02-28  Michael Collison <michael.collison@linaro.org>
>>>>
>>>>      PR target/70008
>>>>      * config/arm/arm.md (*subsi3_carryin): Only match pattern if
>>>>      TARGET_ARM due to 'rsc' instruction alternative.
>>>>      * config/arm/thumb2.md (*thumb2_subsi3_carryin): New pattern.
>>>>
>>>>
>>> The *subsi3_carrying pattern has the arch attribute:
>>>     (set_attr "arch" "*,a")
>>>
>>> That means that the second alternative that generates the RSC
>>> instruction is only enabled
>>> for ARM mode. Do you have a testcase where this doesn't happen and
>>> this pattern generates
>>> the second alternative for Thumb2?
>> No I don't have a test case; i noticed the pattern when working on the
>> overflow project. I did not realize
>> that an attribute could affect the matching of an alternative. I will
>> close the bug.
>>
>>>
>>> Thanks,
>>> Kyrill
> This is all true, but there is a potential performance issue with this
> pattern though, that could lead to sub-optimal code.
>
> The predicate accepts reg-or-int, but in ARM state only simple
> 'const-ok-for-arm' immediates are permitted by the predicates, and in
> thumb code, no immediates are permitted at all.  This could potentially
> result in sub-optimal code due to late splitting of the pattern.  It
> would be better if the predicate understood these limitations and
> restricted immediates accordingly.
>
> R.
>

-- 
Michael Collison
Linaro Toolchain Working Group
michael.collison@linaro.org

Re: [PATCH][ARM] PR target/70008

[Michael Collison]

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

I have attached a new patch which hopefully address Richard's concerns. 
I modified the predicate on operand 1 to to "arm_rhs_operand" to be 
consistent with the constraints. I retained the split into two patterns; 
one for arm and another for thumb2. I thought this was cleaner.

Okay for trunk?

2016-02-28  Michael Collison  <michael.collison@linaro.org>

     PR target/70008
     * config/arm/arm.md (*subsi3_carryin): Change predicate to
     arm_rhs_operand to be consistent with constraints.
     Only allow pattern for TARGET_ARM.
     * config/arm/thumb2.md (*thumb2_subsi3_carryin): New pattern.

On 02/29/2016 08:29 AM, Richard Earnshaw (lists) wrote:
> On 29/02/16 11:21, Michael Collison wrote:
>>
>> On 2/29/2016 4:06 AM, Kyrill Tkachov wrote:
>>> Hi Michael,
>>>
>>> On 29/02/16 04:47, Michael Collison wrote:
>>>> This patches address PR 70008, where a reverse subtract with carry
>>>> instruction can be generated in thumb2 mode. It was tested with no
>>>> regressions in arm and thumb modes on the following targets:
>>>>
>>>> arm-none-linux-gnueabi
>>>> arm-none-linux-gnuabihf
>>>> armeb-none-linux-gnuabihf
>>>> arm-none-eabi
>>>>
>>>> Okay for trunk?
>>>>
>>>> 2016-02-28  Michael Collison <michael.collison@linaro.org>
>>>>
>>>>      PR target/70008
>>>>      * config/arm/arm.md (*subsi3_carryin): Only match pattern if
>>>>      TARGET_ARM due to 'rsc' instruction alternative.
>>>>      * config/arm/thumb2.md (*thumb2_subsi3_carryin): New pattern.
>>>>
>>>>
>>> The *subsi3_carrying pattern has the arch attribute:
>>>     (set_attr "arch" "*,a")
>>>
>>> That means that the second alternative that generates the RSC
>>> instruction is only enabled
>>> for ARM mode. Do you have a testcase where this doesn't happen and
>>> this pattern generates
>>> the second alternative for Thumb2?
>> No I don't have a test case; i noticed the pattern when working on the
>> overflow project. I did not realize
>> that an attribute could affect the matching of an alternative. I will
>> close the bug.
>>
>>>
>>> Thanks,
>>> Kyrill
> This is all true, but there is a potential performance issue with this
> pattern though, that could lead to sub-optimal code.
>
> The predicate accepts reg-or-int, but in ARM state only simple
> 'const-ok-for-arm' immediates are permitted by the predicates, and in
> thumb code, no immediates are permitted at all.  This could potentially
> result in sub-optimal code due to late splitting of the pattern.  It
> would be better if the predicate understood these limitations and
> restricted immediates accordingly.
>
> R.
>

-- 
Michael Collison
Linaro Toolchain Working Group
michael.collison@linaro.org


[-- Attachment #2: bugzilla-70008-upstream-v2.patch --]
[-- Type: text/x-patch, Size: 1709 bytes --]

diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index e67239d..e6bcd7f 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -867,15 +867,14 @@
 
 (define_insn "*subsi3_carryin"
   [(set (match_operand:SI 0 "s_register_operand" "=r,r")
-        (minus:SI (minus:SI (match_operand:SI 1 "reg_or_int_operand" "r,I")
+        (minus:SI (minus:SI (match_operand:SI 1 "arm_rhs_operand" "r,I")
                             (match_operand:SI 2 "s_register_operand" "r,r"))
                   (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))]
-  "TARGET_32BIT"
+  "TARGET_ARM"
   "@
    sbc%?\\t%0, %1, %2
    rsc%?\\t%0, %2, %1"
   [(set_attr "conds" "use")
-   (set_attr "arch" "*,a")
    (set_attr "predicable" "yes")
    (set_attr "predicable_short_it" "no")
    (set_attr "type" "adc_reg,adc_imm")]
diff --git a/gcc/config/arm/thumb2.md b/gcc/config/arm/thumb2.md
index 9925365..79305c5 100644
--- a/gcc/config/arm/thumb2.md
+++ b/gcc/config/arm/thumb2.md
@@ -848,6 +848,20 @@
    (set_attr "type" "multiple")]
 )
 
+(define_insn "*thumb2_subsi3_carryin"
+  [(set (match_operand:SI 0 "s_register_operand" "=r")
+        (minus:SI (minus:SI (match_operand:SI 1 "s_register_operand" "r")
+                            (match_operand:SI 2 "s_register_operand" "r"))
+                  (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))]
+  "TARGET_THUMB2"
+  "@
+   sbc%?\\t%0, %1, %2"
+  [(set_attr "conds" "use")
+   (set_attr "predicable" "yes")
+   (set_attr "predicable_short_it" "no")
+   (set_attr "type" "adc_reg")]
+)
+
 (define_insn "*thumb2_cond_sub"
   [(set (match_operand:SI 0 "s_register_operand" "=Ts,Ts")
         (minus:SI (match_operand:SI 1 "s_register_operand" "0,?Ts")
-- 
1.9.1

Re: [PATCH][ARM] PR target/70008

[Richard Earnshaw (lists)]

On 03/03/16 07:23, Michael Collison wrote:
> I have attached a new patch which hopefully address Richard's concerns.
> I modified the predicate on operand 1 to to "arm_rhs_operand" to be
> consistent with the constraints. I retained the split into two patterns;
> one for arm and another for thumb2. I thought this was cleaner.
> 
> Okay for trunk?
> 
> 2016-02-28  Michael Collison  <michael.collison@linaro.org>
> 
>     PR target/70008
>     * config/arm/arm.md (*subsi3_carryin): Change predicate to
>     arm_rhs_operand to be consistent with constraints.
>     Only allow pattern for TARGET_ARM.
>     * config/arm/thumb2.md (*thumb2_subsi3_carryin): New pattern.
> 

I don't think we need two patterns.  We could share this if we had a new
predicate that was called something like reg_or_arm_rhs_operand,  with a
rule that's something like:

  register_operand (op) || (TARGET_ARM && arm_immediate_operand (op))

R.
> On 02/29/2016 08:29 AM, Richard Earnshaw (lists) wrote:
>> On 29/02/16 11:21, Michael Collison wrote:
>>>
>>> On 2/29/2016 4:06 AM, Kyrill Tkachov wrote:
>>>> Hi Michael,
>>>>
>>>> On 29/02/16 04:47, Michael Collison wrote:
>>>>> This patches address PR 70008, where a reverse subtract with carry
>>>>> instruction can be generated in thumb2 mode. It was tested with no
>>>>> regressions in arm and thumb modes on the following targets:
>>>>>
>>>>> arm-none-linux-gnueabi
>>>>> arm-none-linux-gnuabihf
>>>>> armeb-none-linux-gnuabihf
>>>>> arm-none-eabi
>>>>>
>>>>> Okay for trunk?
>>>>>
>>>>> 2016-02-28  Michael Collison <michael.collison@linaro.org>
>>>>>
>>>>>      PR target/70008
>>>>>      * config/arm/arm.md (*subsi3_carryin): Only match pattern if
>>>>>      TARGET_ARM due to 'rsc' instruction alternative.
>>>>>      * config/arm/thumb2.md (*thumb2_subsi3_carryin): New pattern.
>>>>>
>>>>>
>>>> The *subsi3_carrying pattern has the arch attribute:
>>>>     (set_attr "arch" "*,a")
>>>>
>>>> That means that the second alternative that generates the RSC
>>>> instruction is only enabled
>>>> for ARM mode. Do you have a testcase where this doesn't happen and
>>>> this pattern generates
>>>> the second alternative for Thumb2?
>>> No I don't have a test case; i noticed the pattern when working on the
>>> overflow project. I did not realize
>>> that an attribute could affect the matching of an alternative. I will
>>> close the bug.
>>>
>>>>
>>>> Thanks,
>>>> Kyrill
>> This is all true, but there is a potential performance issue with this
>> pattern though, that could lead to sub-optimal code.
>>
>> The predicate accepts reg-or-int, but in ARM state only simple
>> 'const-ok-for-arm' immediates are permitted by the predicates, and in
>> thumb code, no immediates are permitted at all.  This could potentially
>> result in sub-optimal code due to late splitting of the pattern.  It
>> would be better if the predicate understood these limitations and
>> restricted immediates accordingly.
>>
>> R.
>>
> 
> 
> bugzilla-70008-upstream-v2.patch
> 
> 
> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
> index e67239d..e6bcd7f 100644
> --- a/gcc/config/arm/arm.md
> +++ b/gcc/config/arm/arm.md
> @@ -867,15 +867,14 @@
>  
>  (define_insn "*subsi3_carryin"
>    [(set (match_operand:SI 0 "s_register_operand" "=r,r")
> -        (minus:SI (minus:SI (match_operand:SI 1 "reg_or_int_operand" "r,I")
> +        (minus:SI (minus:SI (match_operand:SI 1 "arm_rhs_operand" "r,I")
>                              (match_operand:SI 2 "s_register_operand" "r,r"))
>                    (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))]
> -  "TARGET_32BIT"
> +  "TARGET_ARM"
>    "@
>     sbc%?\\t%0, %1, %2
>     rsc%?\\t%0, %2, %1"
>    [(set_attr "conds" "use")
> -   (set_attr "arch" "*,a")
>     (set_attr "predicable" "yes")
>     (set_attr "predicable_short_it" "no")
>     (set_attr "type" "adc_reg,adc_imm")]
> diff --git a/gcc/config/arm/thumb2.md b/gcc/config/arm/thumb2.md
> index 9925365..79305c5 100644
> --- a/gcc/config/arm/thumb2.md
> +++ b/gcc/config/arm/thumb2.md
> @@ -848,6 +848,20 @@
>     (set_attr "type" "multiple")]
>  )
>  
> +(define_insn "*thumb2_subsi3_carryin"
> +  [(set (match_operand:SI 0 "s_register_operand" "=r")
> +        (minus:SI (minus:SI (match_operand:SI 1 "s_register_operand" "r")
> +                            (match_operand:SI 2 "s_register_operand" "r"))
> +                  (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))]
> +  "TARGET_THUMB2"
> +  "@
> +   sbc%?\\t%0, %1, %2"
> +  [(set_attr "conds" "use")
> +   (set_attr "predicable" "yes")
> +   (set_attr "predicable_short_it" "no")
> +   (set_attr "type" "adc_reg")]
> +)
> +
>  (define_insn "*thumb2_cond_sub"
>    [(set (match_operand:SI 0 "s_register_operand" "=Ts,Ts")
>          (minus:SI (match_operand:SI 1 "s_register_operand" "0,?Ts")
> 

Re: [PATCH][ARM] PR target/70008

[Michael Collison]

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

New patch that adds the predicate "reg_or_arm_rhs_operand" as suggested 
by Richard. Tested on all arm and thumb targets as before.

Okay for trunk?

2016-03-07  Michael Collison  <michael.collison@linaro.org>

     PR target/70008
     * config/arm/predicates.md (reg_or_arm_rhs_operand): New predicate
     that allows registers or immediates if TARGET_ARM.
     * config/arm/arm.md (*subsi3_carryin): Change predicate to
     reg_or_arm_rhs_operand to be consistent with constraints.

On 03/03/2016 08:47 PM, Richard Earnshaw (lists) wrote:
> On 03/03/16 07:23, Michael Collison wrote:
>> I have attached a new patch which hopefully address Richard's concerns.
>> I modified the predicate on operand 1 to to "arm_rhs_operand" to be
>> consistent with the constraints. I retained the split into two patterns;
>> one for arm and another for thumb2. I thought this was cleaner.
>>
>> Okay for trunk?
>>
>> 2016-02-28  Michael Collison  <michael.collison@linaro.org>
>>
>>      PR target/70008
>>      * config/arm/arm.md (*subsi3_carryin): Change predicate to
>>      arm_rhs_operand to be consistent with constraints.
>>      Only allow pattern for TARGET_ARM.
>>      * config/arm/thumb2.md (*thumb2_subsi3_carryin): New pattern.
>>
> I don't think we need two patterns.  We could share this if we had a new
> predicate that was called something like reg_or_arm_rhs_operand,  with a
> rule that's something like:
>
>    register_operand (op) || (TARGET_ARM && arm_immediate_operand (op))
>
> R.
>> On 02/29/2016 08:29 AM, Richard Earnshaw (lists) wrote:
>>> On 29/02/16 11:21, Michael Collison wrote:
>>>> On 2/29/2016 4:06 AM, Kyrill Tkachov wrote:
>>>>> Hi Michael,
>>>>>
>>>>> On 29/02/16 04:47, Michael Collison wrote:
>>>>>> This patches address PR 70008, where a reverse subtract with carry
>>>>>> instruction can be generated in thumb2 mode. It was tested with no
>>>>>> regressions in arm and thumb modes on the following targets:
>>>>>>
>>>>>> arm-none-linux-gnueabi
>>>>>> arm-none-linux-gnuabihf
>>>>>> armeb-none-linux-gnuabihf
>>>>>> arm-none-eabi
>>>>>>
>>>>>> Okay for trunk?
>>>>>>
>>>>>> 2016-02-28  Michael Collison <michael.collison@linaro.org>
>>>>>>
>>>>>>       PR target/70008
>>>>>>       * config/arm/arm.md (*subsi3_carryin): Only match pattern if
>>>>>>       TARGET_ARM due to 'rsc' instruction alternative.
>>>>>>       * config/arm/thumb2.md (*thumb2_subsi3_carryin): New pattern.
>>>>>>
>>>>>>
>>>>> The *subsi3_carrying pattern has the arch attribute:
>>>>>      (set_attr "arch" "*,a")
>>>>>
>>>>> That means that the second alternative that generates the RSC
>>>>> instruction is only enabled
>>>>> for ARM mode. Do you have a testcase where this doesn't happen and
>>>>> this pattern generates
>>>>> the second alternative for Thumb2?
>>>> No I don't have a test case; i noticed the pattern when working on the
>>>> overflow project. I did not realize
>>>> that an attribute could affect the matching of an alternative. I will
>>>> close the bug.
>>>>
>>>>> Thanks,
>>>>> Kyrill
>>> This is all true, but there is a potential performance issue with this
>>> pattern though, that could lead to sub-optimal code.
>>>
>>> The predicate accepts reg-or-int, but in ARM state only simple
>>> 'const-ok-for-arm' immediates are permitted by the predicates, and in
>>> thumb code, no immediates are permitted at all.  This could potentially
>>> result in sub-optimal code due to late splitting of the pattern.  It
>>> would be better if the predicate understood these limitations and
>>> restricted immediates accordingly.
>>>
>>> R.
>>>
>>
>> bugzilla-70008-upstream-v2.patch
>>
>>
>> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
>> index e67239d..e6bcd7f 100644
>> --- a/gcc/config/arm/arm.md
>> +++ b/gcc/config/arm/arm.md
>> @@ -867,15 +867,14 @@
>>   
>>   (define_insn "*subsi3_carryin"
>>     [(set (match_operand:SI 0 "s_register_operand" "=r,r")
>> -        (minus:SI (minus:SI (match_operand:SI 1 "reg_or_int_operand" "r,I")
>> +        (minus:SI (minus:SI (match_operand:SI 1 "arm_rhs_operand" "r,I")
>>                               (match_operand:SI 2 "s_register_operand" "r,r"))
>>                     (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))]
>> -  "TARGET_32BIT"
>> +  "TARGET_ARM"
>>     "@
>>      sbc%?\\t%0, %1, %2
>>      rsc%?\\t%0, %2, %1"
>>     [(set_attr "conds" "use")
>> -   (set_attr "arch" "*,a")
>>      (set_attr "predicable" "yes")
>>      (set_attr "predicable_short_it" "no")
>>      (set_attr "type" "adc_reg,adc_imm")]
>> diff --git a/gcc/config/arm/thumb2.md b/gcc/config/arm/thumb2.md
>> index 9925365..79305c5 100644
>> --- a/gcc/config/arm/thumb2.md
>> +++ b/gcc/config/arm/thumb2.md
>> @@ -848,6 +848,20 @@
>>      (set_attr "type" "multiple")]
>>   )
>>   
>> +(define_insn "*thumb2_subsi3_carryin"
>> +  [(set (match_operand:SI 0 "s_register_operand" "=r")
>> +        (minus:SI (minus:SI (match_operand:SI 1 "s_register_operand" "r")
>> +                            (match_operand:SI 2 "s_register_operand" "r"))
>> +                  (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))]
>> +  "TARGET_THUMB2"
>> +  "@
>> +   sbc%?\\t%0, %1, %2"
>> +  [(set_attr "conds" "use")
>> +   (set_attr "predicable" "yes")
>> +   (set_attr "predicable_short_it" "no")
>> +   (set_attr "type" "adc_reg")]
>> +)
>> +
>>   (define_insn "*thumb2_cond_sub"
>>     [(set (match_operand:SI 0 "s_register_operand" "=Ts,Ts")
>>           (minus:SI (match_operand:SI 1 "s_register_operand" "0,?Ts")
>>

-- 
Michael Collison
Linaro Toolchain Working Group
michael.collison@linaro.org


[-- Attachment #2: bugzilla-70008-upstream-v3.patch --]
[-- Type: text/x-patch, Size: 1195 bytes --]

diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index e67239d..eb4803a 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -867,7 +867,7 @@
 
 (define_insn "*subsi3_carryin"
   [(set (match_operand:SI 0 "s_register_operand" "=r,r")
-        (minus:SI (minus:SI (match_operand:SI 1 "reg_or_int_operand" "r,I")
+        (minus:SI (minus:SI (match_operand:SI 1 "reg_or_arm_rhs_operand" "r,I")
                             (match_operand:SI 2 "s_register_operand" "r,r"))
                   (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))]
   "TARGET_32BIT"
diff --git a/gcc/config/arm/predicates.md b/gcc/config/arm/predicates.md
index b1cd556..8bf7c1a 100644
--- a/gcc/config/arm/predicates.md
+++ b/gcc/config/arm/predicates.md
@@ -144,6 +144,11 @@
   (and (match_code "const_int")
        (match_test "INTVAL (op) == 0")))
 
+(define_predicate "reg_or_arm_rhs_operand"
+  (ior (match_operand 0 "s_register_operand")
+       (and (match_test "TARGET_ARM")
+	    (match_operand 0 "arm_immediate_operand"))))
+
 ;; Something valid on the RHS of an ARM data-processing instruction
 (define_predicate "arm_rhs_operand"
   (ior (match_operand 0 "s_register_operand")
-- 
1.9.1

Re: [PATCH][ARM] PR target/70008

[Ramana Radhakrishnan]

On Mon, Mar 7, 2016 at 3:51 AM, Michael Collison
<michael.collison@linaro.org> wrote:
> New patch that adds the predicate "reg_or_arm_rhs_operand" as suggested by
> Richard. Tested on all arm and thumb targets as before.
>
> Okay for trunk?

Missing comment on the predicate.

Ok, please queue this for GCC 7 with a comment added. I don't think
this is a "regression" worth fixing for GCC 6.



Thanks,
Ramana

>
> 2016-03-07  Michael Collison  <michael.collison@linaro.org>
>
>     PR target/70008
>     * config/arm/predicates.md (reg_or_arm_rhs_operand): New predicate
>     that allows registers or immediates if TARGET_ARM.
>     * config/arm/arm.md (*subsi3_carryin): Change predicate to
>     reg_or_arm_rhs_operand to be consistent with constraints.
>
>
> On 03/03/2016 08:47 PM, Richard Earnshaw (lists) wrote:
>>
>> On 03/03/16 07:23, Michael Collison wrote:
>>>
>>> I have attached a new patch which hopefully address Richard's concerns.
>>> I modified the predicate on operand 1 to to "arm_rhs_operand" to be
>>> consistent with the constraints. I retained the split into two patterns;
>>> one for arm and another for thumb2. I thought this was cleaner.
>>>
>>> Okay for trunk?
>>>
>>> 2016-02-28  Michael Collison  <michael.collison@linaro.org>
>>>
>>>      PR target/70008
>>>      * config/arm/arm.md (*subsi3_carryin): Change predicate to
>>>      arm_rhs_operand to be consistent with constraints.
>>>      Only allow pattern for TARGET_ARM.
>>>      * config/arm/thumb2.md (*thumb2_subsi3_carryin): New pattern.
>>>
>> I don't think we need two patterns.  We could share this if we had a new
>> predicate that was called something like reg_or_arm_rhs_operand,  with a
>> rule that's something like:
>>
>>    register_operand (op) || (TARGET_ARM && arm_immediate_operand (op))
>>
>> R.
>>>
>>> On 02/29/2016 08:29 AM, Richard Earnshaw (lists) wrote:
>>>>
>>>> On 29/02/16 11:21, Michael Collison wrote:
>>>>>
>>>>> On 2/29/2016 4:06 AM, Kyrill Tkachov wrote:
>>>>>>
>>>>>> Hi Michael,
>>>>>>
>>>>>> On 29/02/16 04:47, Michael Collison wrote:
>>>>>>>
>>>>>>> This patches address PR 70008, where a reverse subtract with carry
>>>>>>> instruction can be generated in thumb2 mode. It was tested with no
>>>>>>> regressions in arm and thumb modes on the following targets:
>>>>>>>
>>>>>>> arm-none-linux-gnueabi
>>>>>>> arm-none-linux-gnuabihf
>>>>>>> armeb-none-linux-gnuabihf
>>>>>>> arm-none-eabi
>>>>>>>
>>>>>>> Okay for trunk?
>>>>>>>
>>>>>>> 2016-02-28  Michael Collison <michael.collison@linaro.org>
>>>>>>>
>>>>>>>       PR target/70008
>>>>>>>       * config/arm/arm.md (*subsi3_carryin): Only match pattern if
>>>>>>>       TARGET_ARM due to 'rsc' instruction alternative.
>>>>>>>       * config/arm/thumb2.md (*thumb2_subsi3_carryin): New pattern.
>>>>>>>
>>>>>>>
>>>>>> The *subsi3_carrying pattern has the arch attribute:
>>>>>>      (set_attr "arch" "*,a")
>>>>>>
>>>>>> That means that the second alternative that generates the RSC
>>>>>> instruction is only enabled
>>>>>> for ARM mode. Do you have a testcase where this doesn't happen and
>>>>>> this pattern generates
>>>>>> the second alternative for Thumb2?
>>>>>
>>>>> No I don't have a test case; i noticed the pattern when working on the
>>>>> overflow project. I did not realize
>>>>> that an attribute could affect the matching of an alternative. I will
>>>>> close the bug.
>>>>>
>>>>>> Thanks,
>>>>>> Kyrill
>>>>
>>>> This is all true, but there is a potential performance issue with this
>>>> pattern though, that could lead to sub-optimal code.
>>>>
>>>> The predicate accepts reg-or-int, but in ARM state only simple
>>>> 'const-ok-for-arm' immediates are permitted by the predicates, and in
>>>> thumb code, no immediates are permitted at all.  This could potentially
>>>> result in sub-optimal code due to late splitting of the pattern.  It
>>>> would be better if the predicate understood these limitations and
>>>> restricted immediates accordingly.
>>>>
>>>> R.
>>>>
>>>
>>> bugzilla-70008-upstream-v2.patch
>>>
>>>
>>> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
>>> index e67239d..e6bcd7f 100644
>>> --- a/gcc/config/arm/arm.md
>>> +++ b/gcc/config/arm/arm.md
>>> @@ -867,15 +867,14 @@
>>>     (define_insn "*subsi3_carryin"
>>>     [(set (match_operand:SI 0 "s_register_operand" "=r,r")
>>> -        (minus:SI (minus:SI (match_operand:SI 1 "reg_or_int_operand"
>>> "r,I")
>>> +        (minus:SI (minus:SI (match_operand:SI 1 "arm_rhs_operand" "r,I")
>>>                               (match_operand:SI 2 "s_register_operand"
>>> "r,r"))
>>>                     (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))]
>>> -  "TARGET_32BIT"
>>> +  "TARGET_ARM"
>>>     "@
>>>      sbc%?\\t%0, %1, %2
>>>      rsc%?\\t%0, %2, %1"
>>>     [(set_attr "conds" "use")
>>> -   (set_attr "arch" "*,a")
>>>      (set_attr "predicable" "yes")
>>>      (set_attr "predicable_short_it" "no")
>>>      (set_attr "type" "adc_reg,adc_imm")]
>>> diff --git a/gcc/config/arm/thumb2.md b/gcc/config/arm/thumb2.md
>>> index 9925365..79305c5 100644
>>> --- a/gcc/config/arm/thumb2.md
>>> +++ b/gcc/config/arm/thumb2.md
>>> @@ -848,6 +848,20 @@
>>>      (set_attr "type" "multiple")]
>>>   )
>>>   +(define_insn "*thumb2_subsi3_carryin"
>>> +  [(set (match_operand:SI 0 "s_register_operand" "=r")
>>> +        (minus:SI (minus:SI (match_operand:SI 1 "s_register_operand"
>>> "r")
>>> +                            (match_operand:SI 2 "s_register_operand"
>>> "r"))
>>> +                  (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))]
>>> +  "TARGET_THUMB2"
>>> +  "@
>>> +   sbc%?\\t%0, %1, %2"
>>> +  [(set_attr "conds" "use")
>>> +   (set_attr "predicable" "yes")
>>> +   (set_attr "predicable_short_it" "no")
>>> +   (set_attr "type" "adc_reg")]
>>> +)
>>> +
>>>   (define_insn "*thumb2_cond_sub"
>>>     [(set (match_operand:SI 0 "s_register_operand" "=Ts,Ts")
>>>           (minus:SI (match_operand:SI 1 "s_register_operand" "0,?Ts")
>>>
>
> --
> Michael Collison
> Linaro Toolchain Working Group
> michael.collison@linaro.org
>