[ARM] PR target/70008

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