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

[Ramana Radhakrishnan <ramana dot gcc at googlemail dot com>]

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
>