diff mbox

[ARM] PR target/70008

Message ID 56D3CD4F.6060301@linaro.org
State Superseded
Headers show

Commit Message

Michael Collison Feb. 29, 2016, 4:47 a.m. UTC
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

Comments

Michael Collison Feb. 29, 2016, 11:21 a.m. UTC | #1
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
Richard Earnshaw (lists) Feb. 29, 2016, 3:29 p.m. UTC | #2
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 March 2, 2016, 7:32 a.m. UTC | #3
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 mbox

Patch

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