[ARM] PR target/70008

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

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")

>