* [PATCH][ARM][2/3] Make if_neg_move and if_move_neg into insn_and_split
@ 2015-07-24 11:10 Kyrill Tkachov
2015-07-31 8:50 ` Kyrill Tkachov
2015-07-31 10:24 ` Ramana Radhakrishnan
0 siblings, 2 replies; 9+ messages in thread
From: Kyrill Tkachov @ 2015-07-24 11:10 UTC (permalink / raw)
To: GCC Patches; +Cc: Ramana Radhakrishnan, Richard Earnshaw
[-- Attachment #1: Type: text/plain, Size: 1094 bytes --]
Hi all,
As part of patch 3/3 I want to expand to an if_then_else form of a conditional negate
operation. It seems we already have patterns for that, but they are ancient and only enabled
for TARGET_ARM. Doing a bit of archaeology I see that they were added in 1997 before Thumb and before
define_insn_and_split. A commit in 2000 that introduced Thumb mode restricted them to only TARGET_ARM.
This patch modernises the patterns by converting them to define_insn_and_splits, removes the redundant
alternatives that can be expressed using constraints and splits them into a cond_exec of a negate operation
after reload (when we're allowed to generate cond_exec rtxes).
This way, we can enable both patterns for TARGET_32BIT i.e. both ARM and Thumb2 states.
I did two bootstraps, one in arm mode, one in Thumb2 mode.
Both were fine.
Tested on arm too.
Ok for trunk?
Thanks,
Kyrill
2015-07-24 Kyrylo Tkachov <kyrylo.tkachov@arm.com>
* config/arm/arm.md (*if_neg_move): Convert to insn_and_split.
Enable for TARGET_32BIT.
(*if_move_neg): Likewise.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: arm-if-neg-move.patch --]
[-- Type: text/x-patch; name=arm-if-neg-move.patch, Size: 3213 bytes --]
commit 1b495b6cb68c77f628e1c1d672c06dcdf5ccf79b
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date: Thu Jul 23 09:20:41 2015 +0100
[ARM] Make if_neg_move and if_move_neg into insn_and_split
diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 0be70a8..f341109 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -10064,21 +10064,24 @@ (define_insn "*ifcompare_neg_move"
(set_attr "type" "multiple")]
)
-(define_insn "*if_neg_move"
- [(set (match_operand:SI 0 "s_register_operand" "=r,r,r")
+(define_insn_and_split "*if_neg_move"
+ [(set (match_operand:SI 0 "s_register_operand" "=l,r")
(if_then_else:SI
(match_operator 4 "arm_comparison_operator"
[(match_operand 3 "cc_register" "") (const_int 0)])
- (neg:SI (match_operand:SI 2 "s_register_operand" "r,r,r"))
- (match_operand:SI 1 "arm_not_operand" "0,?rI,K")))]
- "TARGET_ARM"
- "@
- rsb%d4\\t%0, %2, #0
- mov%D4\\t%0, %1\;rsb%d4\\t%0, %2, #0
- mvn%D4\\t%0, #%B1\;rsb%d4\\t%0, %2, #0"
+ (neg:SI (match_operand:SI 2 "s_register_operand" "l,r"))
+ (match_operand:SI 1 "arm_not_operand" "0,0")))]
+ "TARGET_32BIT"
+ "#"
+ "&& reload_completed"
+ [(cond_exec (match_op_dup 4 [(match_dup 3) (const_int 0)])
+ (set (match_dup 0) (neg:SI (match_dup 2))))]
+ ""
[(set_attr "conds" "use")
- (set_attr "length" "4,8,8")
- (set_attr "type" "logic_shift_imm,multiple,multiple")]
+ (set_attr "length" "4")
+ (set_attr "arch" "t2,32")
+ (set_attr "enabled_for_depr_it" "yes,no")
+ (set_attr "type" "logic_shift_imm")]
)
(define_insn "*ifcompare_move_neg"
@@ -10097,21 +10100,34 @@ (define_insn "*ifcompare_move_neg"
(set_attr "type" "multiple")]
)
-(define_insn "*if_move_neg"
- [(set (match_operand:SI 0 "s_register_operand" "=r,r,r")
+(define_insn_and_split "*if_move_neg"
+ [(set (match_operand:SI 0 "s_register_operand" "=l,r")
(if_then_else:SI
(match_operator 4 "arm_comparison_operator"
[(match_operand 3 "cc_register" "") (const_int 0)])
- (match_operand:SI 1 "arm_not_operand" "0,?rI,K")
- (neg:SI (match_operand:SI 2 "s_register_operand" "r,r,r"))))]
- "TARGET_ARM"
- "@
- rsb%D4\\t%0, %2, #0
- mov%d4\\t%0, %1\;rsb%D4\\t%0, %2, #0
- mvn%d4\\t%0, #%B1\;rsb%D4\\t%0, %2, #0"
+ (match_operand:SI 1 "arm_not_operand" "0,0")
+ (neg:SI (match_operand:SI 2 "s_register_operand" "l,r"))))]
+ "TARGET_32BIT"
+ "#"
+ "&& reload_completed"
+ [(cond_exec (match_dup 5)
+ (set (match_dup 0) (neg:SI (match_dup 2))))]
+ {
+ machine_mode mode = GET_MODE (operands[3]);
+ rtx_code rc = GET_CODE (operands[4]);
+
+ if (mode == CCFPmode || mode == CCFPEmode)
+ rc = reverse_condition_maybe_unordered (rc);
+ else
+ rc = reverse_condition (rc);
+
+ operands[5] = gen_rtx_fmt_ee (rc, VOIDmode, operands[3], const0_rtx);
+ }
[(set_attr "conds" "use")
- (set_attr "length" "4,8,8")
- (set_attr "type" "logic_shift_imm,multiple,multiple")]
+ (set_attr "length" "4")
+ (set_attr "arch" "t2,32")
+ (set_attr "enabled_for_depr_it" "yes,no")
+ (set_attr "type" "logic_shift_imm")]
)
(define_insn "*arith_adjacentmem"
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][ARM][2/3] Make if_neg_move and if_move_neg into insn_and_split
2015-07-24 11:10 [PATCH][ARM][2/3] Make if_neg_move and if_move_neg into insn_and_split Kyrill Tkachov
@ 2015-07-31 8:50 ` Kyrill Tkachov
2015-07-31 10:24 ` Ramana Radhakrishnan
1 sibling, 0 replies; 9+ messages in thread
From: Kyrill Tkachov @ 2015-07-31 8:50 UTC (permalink / raw)
To: GCC Patches; +Cc: Ramana Radhakrishnan, Richard Earnshaw
Ping.
https://gcc.gnu.org/ml/gcc-patches/2015-07/msg02038.html
This is a prerequisite for patch 3/3 but is a useful cleanup in its own right.
Thanks,
Kyrill
On 24/07/15 11:55, Kyrill Tkachov wrote:
> Hi all,
>
> As part of patch 3/3 I want to expand to an if_then_else form of a conditional negate
> operation. It seems we already have patterns for that, but they are ancient and only enabled
> for TARGET_ARM. Doing a bit of archaeology I see that they were added in 1997 before Thumb and before
> define_insn_and_split. A commit in 2000 that introduced Thumb mode restricted them to only TARGET_ARM.
>
> This patch modernises the patterns by converting them to define_insn_and_splits, removes the redundant
> alternatives that can be expressed using constraints and splits them into a cond_exec of a negate operation
> after reload (when we're allowed to generate cond_exec rtxes).
>
> This way, we can enable both patterns for TARGET_32BIT i.e. both ARM and Thumb2 states.
>
> I did two bootstraps, one in arm mode, one in Thumb2 mode.
> Both were fine.
> Tested on arm too.
>
> Ok for trunk?
>
> Thanks,
> Kyrill
>
> 2015-07-24 Kyrylo Tkachov <kyrylo.tkachov@arm.com>
>
> * config/arm/arm.md (*if_neg_move): Convert to insn_and_split.
> Enable for TARGET_32BIT.
> (*if_move_neg): Likewise.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][ARM][2/3] Make if_neg_move and if_move_neg into insn_and_split
2015-07-24 11:10 [PATCH][ARM][2/3] Make if_neg_move and if_move_neg into insn_and_split Kyrill Tkachov
2015-07-31 8:50 ` Kyrill Tkachov
@ 2015-07-31 10:24 ` Ramana Radhakrishnan
2015-07-31 10:34 ` Kyrill Tkachov
1 sibling, 1 reply; 9+ messages in thread
From: Ramana Radhakrishnan @ 2015-07-31 10:24 UTC (permalink / raw)
To: Kyrill Tkachov, GCC Patches; +Cc: Ramana Radhakrishnan, Richard Earnshaw
Hmmm, not sure if this is that straightforward just looking at this..
> 2015-07-24 Kyrylo Tkachov <kyrylo.tkachov@arm.com>
>
> * config/arm/arm.md (*if_neg_move): Convert to insn_and_split.
> Enable for TARGET_32BIT.
> (*if_move_neg): Likewise.
> commit 1b495b6cb68c77f628e1c1d672c06dcdf5ccf79b
> Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
> Date: Thu Jul 23 09:20:41 2015 +0100
>
> [ARM] Make if_neg_move and if_move_neg into insn_and_split
>
> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
> index 0be70a8..f341109 100644
> --- a/gcc/config/arm/arm.md
> +++ b/gcc/config/arm/arm.md
> @@ -10064,21 +10064,24 @@ (define_insn "*ifcompare_neg_move"
> (set_attr "type" "multiple")]
> )
>
> -(define_insn "*if_neg_move"
> - [(set (match_operand:SI 0 "s_register_operand" "=r,r,r")
> +(define_insn_and_split "*if_neg_move"
> + [(set (match_operand:SI 0 "s_register_operand" "=l,r")
> (if_then_else:SI
> (match_operator 4 "arm_comparison_operator"
> [(match_operand 3 "cc_register" "") (const_int 0)])
> - (neg:SI (match_operand:SI 2 "s_register_operand" "r,r,r"))
> - (match_operand:SI 1 "arm_not_operand" "0,?rI,K")))]
> - "TARGET_ARM"
> - "@
> - rsb%d4\\t%0, %2, #0
> - mov%D4\\t%0, %1\;rsb%d4\\t%0, %2, #0
> - mvn%D4\\t%0, #%B1\;rsb%d4\\t%0, %2, #0"
> + (neg:SI (match_operand:SI 2 "s_register_operand" "l,r"))
> + (match_operand:SI 1 "arm_not_operand" "0,0")))]
So, we have a predicate that doesn't cover all the constraints - in this case aren't we forcing everything into operand0. What happens if we just delete this pattern instead of turning it into an insn_and_split - after all we have other parts of the backend where conditional negates and conditional moves will be caught and cond-exec probably post dates some of these if-then-else patterns.
> + "TARGET_32BIT"
> + "#"
> + "&& reload_completed"
> + [(cond_exec (match_op_dup 4 [(match_dup 3) (const_int 0)])
> + (set (match_dup 0) (neg:SI (match_dup 2))))]
> + ""
> [(set_attr "conds" "use")
> - (set_attr "length" "4,8,8")
> - (set_attr "type" "logic_shift_imm,multiple,multiple")]
> + (set_attr "length" "4")
> + (set_attr "arch" "t2,32")
> + (set_attr "enabled_for_depr_it" "yes,no")
> + (set_attr "type" "logic_shift_imm")]
> )
>
> (define_insn "*ifcompare_move_neg"
> @@ -10097,21 +10100,34 @@ (define_insn "*ifcompare_move_neg"
> (set_attr "type" "multiple")]
> )
>
> -(define_insn "*if_move_neg"
> - [(set (match_operand:SI 0 "s_register_operand" "=r,r,r")
> +(define_insn_and_split "*if_move_neg"
> + [(set (match_operand:SI 0 "s_register_operand" "=l,r")
> (if_then_else:SI
> (match_operator 4 "arm_comparison_operator"
> [(match_operand 3 "cc_register" "") (const_int 0)])
> - (match_operand:SI 1 "arm_not_operand" "0,?rI,K")
> - (neg:SI (match_operand:SI 2 "s_register_operand" "r,r,r"))))]
> - "TARGET_ARM"
> - "@
> - rsb%D4\\t%0, %2, #0
> - mov%d4\\t%0, %1\;rsb%D4\\t%0, %2, #0
> - mvn%d4\\t%0, #%B1\;rsb%D4\\t%0, %2, #0"
> + (match_operand:SI 1 "arm_not_operand" "0,0")
> + (neg:SI (match_operand:SI 2 "s_register_operand" "l,r"))))]
> + "TARGET_32BIT"
> + "#"
> + "&& reload_completed"
> + [(cond_exec (match_dup 5)
> + (set (match_dup 0) (neg:SI (match_dup 2))))]
> + {
> + machine_mode mode = GET_MODE (operands[3]);
> + rtx_code rc = GET_CODE (operands[4]);
> +
> + if (mode == CCFPmode || mode == CCFPEmode)
> + rc = reverse_condition_maybe_unordered (rc);
> + else
> + rc = reverse_condition (rc);
> +
> + operands[5] = gen_rtx_fmt_ee (rc, VOIDmode, operands[3], const0_rtx);
> + }
> [(set_attr "conds" "use")
> - (set_attr "length" "4,8,8")
> - (set_attr "type" "logic_shift_imm,multiple,multiple")]
> + (set_attr "length" "4")
> + (set_attr "arch" "t2,32")
> + (set_attr "enabled_for_depr_it" "yes,no")
> + (set_attr "type" "logic_shift_imm")]
> )
Same as above.
>
> (define_insn "*arith_adjacentmem"
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][ARM][2/3] Make if_neg_move and if_move_neg into insn_and_split
2015-07-31 10:24 ` Ramana Radhakrishnan
@ 2015-07-31 10:34 ` Kyrill Tkachov
2015-07-31 10:54 ` Ramana Radhakrishnan
0 siblings, 1 reply; 9+ messages in thread
From: Kyrill Tkachov @ 2015-07-31 10:34 UTC (permalink / raw)
To: Ramana Radhakrishnan, GCC Patches; +Cc: Ramana Radhakrishnan, Richard Earnshaw
On 31/07/15 11:18, Ramana Radhakrishnan wrote:
> Hmmm, not sure if this is that straightforward just looking at this..
>
>> 2015-07-24 Kyrylo Tkachov <kyrylo.tkachov@arm.com>
>>
>> * config/arm/arm.md (*if_neg_move): Convert to insn_and_split.
>> Enable for TARGET_32BIT.
>> (*if_move_neg): Likewise.
>> commit 1b495b6cb68c77f628e1c1d672c06dcdf5ccf79b
>> Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
>> Date: Thu Jul 23 09:20:41 2015 +0100
>> [ARM] Make if_neg_move and if_move_neg into insn_and_split
>> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
>> index 0be70a8..f341109 100644
>> --- a/gcc/config/arm/arm.md
>> +++ b/gcc/config/arm/arm.md
>> @@ -10064,21 +10064,24 @@ (define_insn "*ifcompare_neg_move"
>> (set_attr "type" "multiple")]
>> )
>>
>> -(define_insn "*if_neg_move"
>> - [(set (match_operand:SI 0 "s_register_operand" "=r,r,r")
>> +(define_insn_and_split "*if_neg_move"
>> + [(set (match_operand:SI 0 "s_register_operand" "=l,r")
>> (if_then_else:SI
>> (match_operator 4 "arm_comparison_operator"
>> [(match_operand 3 "cc_register" "") (const_int 0)])
>> - (neg:SI (match_operand:SI 2 "s_register_operand" "r,r,r"))
>> - (match_operand:SI 1 "arm_not_operand" "0,?rI,K")))]
>> - "TARGET_ARM"
>> - "@
>> - rsb%d4\\t%0, %2, #0
>> - mov%D4\\t%0, %1\;rsb%d4\\t%0, %2, #0
>> - mvn%D4\\t%0, #%B1\;rsb%d4\\t%0, %2, #0"
>> + (neg:SI (match_operand:SI 2 "s_register_operand" "l,r"))
>> + (match_operand:SI 1 "arm_not_operand" "0,0")))]
>
>
> So, we have a predicate that doesn't cover all the constraints - in this case aren't we forcing everything into operand0. What happens if we just delete this pattern instead of turning it into an insn_and_split - after all we have other parts of the backend where conditional negates and conditional moves will be caught and cond-exec probably post dates some of these if-then-else patterns.
Hmmm yes, I think operand 1 should be tightened to s_register_operand.
The reason I want this pattern is so that I can expand to it in patch 3/3 where I want to create
a conditional negate expression. However, I can't just emit a COND_EXEC at expand time. I found that
reload doesn't handle the dataflow through them properly. With this pattern I can carry the if_then_else
around and split it into the conditional negate only after reload when it's safe.
Kyrill
>
>
>> + "TARGET_32BIT"
>> + "#"
>> + "&& reload_completed"
>> + [(cond_exec (match_op_dup 4 [(match_dup 3) (const_int 0)])
>> + (set (match_dup 0) (neg:SI (match_dup 2))))]
>> + ""
>> [(set_attr "conds" "use")
>> - (set_attr "length" "4,8,8")
>> - (set_attr "type" "logic_shift_imm,multiple,multiple")]
>> + (set_attr "length" "4")
>> + (set_attr "arch" "t2,32")
>> + (set_attr "enabled_for_depr_it" "yes,no")
>> + (set_attr "type" "logic_shift_imm")]
>> )
>>
>> (define_insn "*ifcompare_move_neg"
>> @@ -10097,21 +10100,34 @@ (define_insn "*ifcompare_move_neg"
>> (set_attr "type" "multiple")]
>> )
>>
>> -(define_insn "*if_move_neg"
>> - [(set (match_operand:SI 0 "s_register_operand" "=r,r,r")
>> +(define_insn_and_split "*if_move_neg"
>> + [(set (match_operand:SI 0 "s_register_operand" "=l,r")
>> (if_then_else:SI
>> (match_operator 4 "arm_comparison_operator"
>> [(match_operand 3 "cc_register" "") (const_int 0)])
>> - (match_operand:SI 1 "arm_not_operand" "0,?rI,K")
>> - (neg:SI (match_operand:SI 2 "s_register_operand" "r,r,r"))))]
>> - "TARGET_ARM"
>> - "@
>> - rsb%D4\\t%0, %2, #0
>> - mov%d4\\t%0, %1\;rsb%D4\\t%0, %2, #0
>> - mvn%d4\\t%0, #%B1\;rsb%D4\\t%0, %2, #0"
>> + (match_operand:SI 1 "arm_not_operand" "0,0")
>> + (neg:SI (match_operand:SI 2 "s_register_operand" "l,r"))))]
>> + "TARGET_32BIT"
>> + "#"
>> + "&& reload_completed"
>> + [(cond_exec (match_dup 5)
>> + (set (match_dup 0) (neg:SI (match_dup 2))))]
>> + {
>> + machine_mode mode = GET_MODE (operands[3]);
>> + rtx_code rc = GET_CODE (operands[4]);
>> +
>> + if (mode == CCFPmode || mode == CCFPEmode)
>> + rc = reverse_condition_maybe_unordered (rc);
>> + else
>> + rc = reverse_condition (rc);
>> +
>> + operands[5] = gen_rtx_fmt_ee (rc, VOIDmode, operands[3], const0_rtx);
>> + }
>> [(set_attr "conds" "use")
>> - (set_attr "length" "4,8,8")
>> - (set_attr "type" "logic_shift_imm,multiple,multiple")]
>> + (set_attr "length" "4")
>> + (set_attr "arch" "t2,32")
>> + (set_attr "enabled_for_depr_it" "yes,no")
>> + (set_attr "type" "logic_shift_imm")]
>> )
>
> Same as above.
>
>>
>> (define_insn "*arith_adjacentmem"
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][ARM][2/3] Make if_neg_move and if_move_neg into insn_and_split
2015-07-31 10:34 ` Kyrill Tkachov
@ 2015-07-31 10:54 ` Ramana Radhakrishnan
2015-07-31 10:58 ` Kyrill Tkachov
0 siblings, 1 reply; 9+ messages in thread
From: Ramana Radhakrishnan @ 2015-07-31 10:54 UTC (permalink / raw)
To: Kyrill Tkachov, GCC Patches; +Cc: Richard Earnshaw
>>
>>
>> So, we have a predicate that doesn't cover all the constraints - in this case aren't we forcing everything into operand0. What happens if we just delete this pattern instead of turning it into an insn_and_split - after all we have other parts of the backend where conditional negates and conditional moves will be caught and cond-exec probably post dates some of these if-then-else patterns.
>
> Hmmm yes, I think operand 1 should be tightened to s_register_operand.
> The reason I want this pattern is so that I can expand to it in patch 3/3 where I want to create
> a conditional negate expression. However, I can't just emit a COND_EXEC at expand time. I found that
> reload doesn't handle the dataflow through them properly. With this pattern I can carry the if_then_else
> around and split it into the conditional negate only after reload when it's safe.
But don't we loose because the immediate alternatives have been lost ? i.e. the original pattern allowed us to express conditional negates where the else condition was a move of an immediate. Thus one didn't require an additional register. Or are you arguing that this is no longer required ?
Not enough coffee in system yet.
regards
Ramana
>
> Kyrill
>
>>
>>
>>> + "TARGET_32BIT"
>>> + "#"
>>> + "&& reload_completed"
>>> + [(cond_exec (match_op_dup 4 [(match_dup 3) (const_int 0)])
>>> + (set (match_dup 0) (neg:SI (match_dup 2))))]
>>> + ""
>>> [(set_attr "conds" "use")
>>> - (set_attr "length" "4,8,8")
>>> - (set_attr "type" "logic_shift_imm,multiple,multiple")]
>>> + (set_attr "length" "4")
>>> + (set_attr "arch" "t2,32")
>>> + (set_attr "enabled_for_depr_it" "yes,no")
>>> + (set_attr "type" "logic_shift_imm")]
>>> )
>>>
>>> (define_insn "*ifcompare_move_neg"
>>> @@ -10097,21 +10100,34 @@ (define_insn "*ifcompare_move_neg"
>>> (set_attr "type" "multiple")]
>>> )
>>>
>>> -(define_insn "*if_move_neg"
>>> - [(set (match_operand:SI 0 "s_register_operand" "=r,r,r")
>>> +(define_insn_and_split "*if_move_neg"
>>> + [(set (match_operand:SI 0 "s_register_operand" "=l,r")
>>> (if_then_else:SI
>>> (match_operator 4 "arm_comparison_operator"
>>> [(match_operand 3 "cc_register" "") (const_int 0)])
>>> - (match_operand:SI 1 "arm_not_operand" "0,?rI,K")
>>> - (neg:SI (match_operand:SI 2 "s_register_operand" "r,r,r"))))]
>>> - "TARGET_ARM"
>>> - "@
>>> - rsb%D4\\t%0, %2, #0
>>> - mov%d4\\t%0, %1\;rsb%D4\\t%0, %2, #0
>>> - mvn%d4\\t%0, #%B1\;rsb%D4\\t%0, %2, #0"
>>> + (match_operand:SI 1 "arm_not_operand" "0,0")
>>> + (neg:SI (match_operand:SI 2 "s_register_operand" "l,r"))))]
>>> + "TARGET_32BIT"
>>> + "#"
>>> + "&& reload_completed"
>>> + [(cond_exec (match_dup 5)
>>> + (set (match_dup 0) (neg:SI (match_dup 2))))]
>>> + {
>>> + machine_mode mode = GET_MODE (operands[3]);
>>> + rtx_code rc = GET_CODE (operands[4]);
>>> +
>>> + if (mode == CCFPmode || mode == CCFPEmode)
>>> + rc = reverse_condition_maybe_unordered (rc);
>>> + else
>>> + rc = reverse_condition (rc);
>>> +
>>> + operands[5] = gen_rtx_fmt_ee (rc, VOIDmode, operands[3], const0_rtx);
>>> + }
>>> [(set_attr "conds" "use")
>>> - (set_attr "length" "4,8,8")
>>> - (set_attr "type" "logic_shift_imm,multiple,multiple")]
>>> + (set_attr "length" "4")
>>> + (set_attr "arch" "t2,32")
>>> + (set_attr "enabled_for_depr_it" "yes,no")
>>> + (set_attr "type" "logic_shift_imm")]
>>> )
>>
>> Same as above.
>>
>>>
>>> (define_insn "*arith_adjacentmem"
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][ARM][2/3] Make if_neg_move and if_move_neg into insn_and_split
2015-07-31 10:54 ` Ramana Radhakrishnan
@ 2015-07-31 10:58 ` Kyrill Tkachov
2015-07-31 11:06 ` Ramana Radhakrishnan
0 siblings, 1 reply; 9+ messages in thread
From: Kyrill Tkachov @ 2015-07-31 10:58 UTC (permalink / raw)
To: Ramana Radhakrishnan, GCC Patches; +Cc: Richard Earnshaw
On 31/07/15 11:34, Ramana Radhakrishnan wrote:
>>>
>>> So, we have a predicate that doesn't cover all the constraints - in this case aren't we forcing everything into operand0. What happens if we just delete this pattern instead of turning it into an insn_and_split - after all we have other parts of the backend where conditional negates and conditional moves will be caught and cond-exec probably post dates some of these if-then-else patterns.
>> Hmmm yes, I think operand 1 should be tightened to s_register_operand.
>> The reason I want this pattern is so that I can expand to it in patch 3/3 where I want to create
>> a conditional negate expression. However, I can't just emit a COND_EXEC at expand time. I found that
>> reload doesn't handle the dataflow through them properly. With this pattern I can carry the if_then_else
>> around and split it into the conditional negate only after reload when it's safe.
> But don't we loose because the immediate alternatives have been lost ? i.e. the original pattern allowed us to express conditional negates where the else condition was a move of an immediate. Thus one didn't require an additional register. Or are you arguing that this is no longer required ?
I am arguing that this is no longer required. In the original pattern the cases where operand 1 is an
immediate just outputs:
mov%D4\\t%0, %1\;rsb%d4\\t%0, %2, #0
or
mvn%D4\\t%0, #%B1\;rsb%d4\\t%0, %2, #0
It doesn't do anything smart.
I can build SPEC2006 with and without this patch to check for suspect code differences, but I suspect
there won't be much that matches it.
For example, even without this patch it doesn't get used for:
int
foo (int a)
{
return a > 5 ? -a : 8;
}
Bottom line: I'll have a look at codegen differences in SPEC to see if there's codegen degradation
Thanks,
Kyrill
>
>
> Not enough coffee in system yet.
>
> regards
> Ramana
>
>> Kyrill
>>
>>>
>>>> + "TARGET_32BIT"
>>>> + "#"
>>>> + "&& reload_completed"
>>>> + [(cond_exec (match_op_dup 4 [(match_dup 3) (const_int 0)])
>>>> + (set (match_dup 0) (neg:SI (match_dup 2))))]
>>>> + ""
>>>> [(set_attr "conds" "use")
>>>> - (set_attr "length" "4,8,8")
>>>> - (set_attr "type" "logic_shift_imm,multiple,multiple")]
>>>> + (set_attr "length" "4")
>>>> + (set_attr "arch" "t2,32")
>>>> + (set_attr "enabled_for_depr_it" "yes,no")
>>>> + (set_attr "type" "logic_shift_imm")]
>>>> )
>>>>
>>>> (define_insn "*ifcompare_move_neg"
>>>> @@ -10097,21 +10100,34 @@ (define_insn "*ifcompare_move_neg"
>>>> (set_attr "type" "multiple")]
>>>> )
>>>>
>>>> -(define_insn "*if_move_neg"
>>>> - [(set (match_operand:SI 0 "s_register_operand" "=r,r,r")
>>>> +(define_insn_and_split "*if_move_neg"
>>>> + [(set (match_operand:SI 0 "s_register_operand" "=l,r")
>>>> (if_then_else:SI
>>>> (match_operator 4 "arm_comparison_operator"
>>>> [(match_operand 3 "cc_register" "") (const_int 0)])
>>>> - (match_operand:SI 1 "arm_not_operand" "0,?rI,K")
>>>> - (neg:SI (match_operand:SI 2 "s_register_operand" "r,r,r"))))]
>>>> - "TARGET_ARM"
>>>> - "@
>>>> - rsb%D4\\t%0, %2, #0
>>>> - mov%d4\\t%0, %1\;rsb%D4\\t%0, %2, #0
>>>> - mvn%d4\\t%0, #%B1\;rsb%D4\\t%0, %2, #0"
>>>> + (match_operand:SI 1 "arm_not_operand" "0,0")
>>>> + (neg:SI (match_operand:SI 2 "s_register_operand" "l,r"))))]
>>>> + "TARGET_32BIT"
>>>> + "#"
>>>> + "&& reload_completed"
>>>> + [(cond_exec (match_dup 5)
>>>> + (set (match_dup 0) (neg:SI (match_dup 2))))]
>>>> + {
>>>> + machine_mode mode = GET_MODE (operands[3]);
>>>> + rtx_code rc = GET_CODE (operands[4]);
>>>> +
>>>> + if (mode == CCFPmode || mode == CCFPEmode)
>>>> + rc = reverse_condition_maybe_unordered (rc);
>>>> + else
>>>> + rc = reverse_condition (rc);
>>>> +
>>>> + operands[5] = gen_rtx_fmt_ee (rc, VOIDmode, operands[3], const0_rtx);
>>>> + }
>>>> [(set_attr "conds" "use")
>>>> - (set_attr "length" "4,8,8")
>>>> - (set_attr "type" "logic_shift_imm,multiple,multiple")]
>>>> + (set_attr "length" "4")
>>>> + (set_attr "arch" "t2,32")
>>>> + (set_attr "enabled_for_depr_it" "yes,no")
>>>> + (set_attr "type" "logic_shift_imm")]
>>>> )
>>> Same as above.
>>>
>>>>
>>>> (define_insn "*arith_adjacentmem"
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][ARM][2/3] Make if_neg_move and if_move_neg into insn_and_split
2015-07-31 10:58 ` Kyrill Tkachov
@ 2015-07-31 11:06 ` Ramana Radhakrishnan
2015-07-31 11:17 ` Kyrill Tkachov
0 siblings, 1 reply; 9+ messages in thread
From: Ramana Radhakrishnan @ 2015-07-31 11:06 UTC (permalink / raw)
To: Kyrill Tkachov, GCC Patches; +Cc: Richard Earnshaw
On 31/07/15 11:49, Kyrill Tkachov wrote:
>
> On 31/07/15 11:34, Ramana Radhakrishnan wrote:
>>>>
>>>> So, we have a predicate that doesn't cover all the constraints - in this case aren't we forcing everything into operand0. What happens if we just delete this pattern instead of turning it into an insn_and_split - after all we have other parts of the backend where conditional negates and conditional moves will be caught and cond-exec probably post dates some of these if-then-else patterns.
>>> Hmmm yes, I think operand 1 should be tightened to s_register_operand.
>>> The reason I want this pattern is so that I can expand to it in patch 3/3 where I want to create
>>> a conditional negate expression. However, I can't just emit a COND_EXEC at expand time. I found that
>>> reload doesn't handle the dataflow through them properly. With this pattern I can carry the if_then_else
>>> around and split it into the conditional negate only after reload when it's safe.
>> But don't we loose because the immediate alternatives have been lost ? i.e. the original pattern allowed us to express conditional negates where the else condition was a move of an immediate. Thus one didn't require an additional register. Or are you arguing that this is no longer required ?
>
> I am arguing that this is no longer required. In the original pattern the cases where operand 1 is an
> immediate just outputs:
>
> mov%D4\\t%0, %1\;rsb%d4\\t%0, %2, #0
> or
> mvn%D4\\t%0, #%B1\;rsb%d4\\t%0, %2, #0
As I said not enough coffee ;) You'll end up getting an unconditional move followed by a conditional neg, so not terrible but may be a bit more work for LRA todo.
>
> It doesn't do anything smart.
> I can build SPEC2006 with and without this patch to check for suspect code differences, but I suspect
> there won't be much that matches it.
>
A sanity check is fine - modulo that it's ok to go in.
regards
Ramana
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][ARM][2/3] Make if_neg_move and if_move_neg into insn_and_split
2015-07-31 11:06 ` Ramana Radhakrishnan
@ 2015-07-31 11:17 ` Kyrill Tkachov
2015-07-31 12:12 ` Ramana Radhakrishnan
0 siblings, 1 reply; 9+ messages in thread
From: Kyrill Tkachov @ 2015-07-31 11:17 UTC (permalink / raw)
To: Ramana Radhakrishnan, GCC Patches; +Cc: Richard Earnshaw
On 31/07/15 12:04, Ramana Radhakrishnan wrote:
>
> On 31/07/15 11:49, Kyrill Tkachov wrote:
>> On 31/07/15 11:34, Ramana Radhakrishnan wrote:
>>>>> So, we have a predicate that doesn't cover all the constraints - in this case aren't we forcing everything into operand0. What happens if we just delete this pattern instead of turning it into an insn_and_split - after all we have other parts of the backend where conditional negates and conditional moves will be caught and cond-exec probably post dates some of these if-then-else patterns.
>>>> Hmmm yes, I think operand 1 should be tightened to s_register_operand.
>>>> The reason I want this pattern is so that I can expand to it in patch 3/3 where I want to create
>>>> a conditional negate expression. However, I can't just emit a COND_EXEC at expand time. I found that
>>>> reload doesn't handle the dataflow through them properly. With this pattern I can carry the if_then_else
>>>> around and split it into the conditional negate only after reload when it's safe.
>>> But don't we loose because the immediate alternatives have been lost ? i.e. the original pattern allowed us to express conditional negates where the else condition was a move of an immediate. Thus one didn't require an additional register. Or are you arguing that this is no longer required ?
>> I am arguing that this is no longer required. In the original pattern the cases where operand 1 is an
>> immediate just outputs:
>>
>> mov%D4\\t%0, %1\;rsb%d4\\t%0, %2, #0
>> or
>> mvn%D4\\t%0, #%B1\;rsb%d4\\t%0, %2, #0
> As I said not enough coffee ;) You'll end up getting an unconditional move followed by a conditional neg, so not terrible but may be a bit more work for LRA todo.
>
>> It doesn't do anything smart.
>> I can build SPEC2006 with and without this patch to check for suspect code differences, but I suspect
>> there won't be much that matches it.
>>
> A sanity check is fine - modulo that it's ok to go in.
As mentioned earlier I'll change operand 1 to be use the s_register_operand predicate and re-test.
Is that change ok as well?
Thanks,
Kyrill
>
>
> regards
> Ramana
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][ARM][2/3] Make if_neg_move and if_move_neg into insn_and_split
2015-07-31 11:17 ` Kyrill Tkachov
@ 2015-07-31 12:12 ` Ramana Radhakrishnan
0 siblings, 0 replies; 9+ messages in thread
From: Ramana Radhakrishnan @ 2015-07-31 12:12 UTC (permalink / raw)
To: Kyrill Tkachov; +Cc: Ramana Radhakrishnan, GCC Patches, Richard Earnshaw
>
>
> As mentioned earlier I'll change operand 1 to be use the s_register_operand
> predicate and re-test.
> Is that change ok as well?
Yes that's ok .
Ramana
>
> Thanks,
> Kyrill
>
>>
>>
>> regards
>> Ramana
>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-07-31 11:31 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-24 11:10 [PATCH][ARM][2/3] Make if_neg_move and if_move_neg into insn_and_split Kyrill Tkachov
2015-07-31 8:50 ` Kyrill Tkachov
2015-07-31 10:24 ` Ramana Radhakrishnan
2015-07-31 10:34 ` Kyrill Tkachov
2015-07-31 10:54 ` Ramana Radhakrishnan
2015-07-31 10:58 ` Kyrill Tkachov
2015-07-31 11:06 ` Ramana Radhakrishnan
2015-07-31 11:17 ` Kyrill Tkachov
2015-07-31 12:12 ` Ramana Radhakrishnan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).