[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.
>

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