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.colli...@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.colli...@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") >
