On Mon, Mar 7, 2016 at 3:51 AM, Michael Collison <michael.colli...@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.colli...@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.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") >>> > > -- > Michael Collison > Linaro Toolchain Working Group > michael.colli...@linaro.org >