| Message ID | 56D3CD4F.6060301@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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
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.
Hi Richard, I think we could incorporate your feedback by changing the predicate on operand 1 to "arm_rhs_operand" which allows "s_register_operand" or "arm_immediate_operand". Everything else in my patch would stay the same including splitting the thumb2 pattern out into it's own insn. I'm testing this change now. Let me know if this direction is okay with you. 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
diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md index e67239d..a008207 100644 --- a/gcc/config/arm/arm.md +++ b/gcc/config/arm/arm.md @@ -870,7 +870,7 @@ (minus:SI (minus:SI (match_operand:SI 1 "reg_or_int_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" 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") -- 1.9.1
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. -- Michael Collison Linaro Toolchain Working Group michael.collison@linaro.org