* [PATCH, ARM] Alternatives and type attributes fixes.
@ 2015-04-27 12:25 Yvan Roux
2015-04-27 13:20 ` Kyrill Tkachov
0 siblings, 1 reply; 5+ messages in thread
From: Yvan Roux @ 2015-04-27 12:25 UTC (permalink / raw)
To: gcc-patches, Ramana Radhakrishnan, Richard Earnshaw, Kyrylo Tkachov
[-- Attachment #1: Type: text/plain, Size: 1427 bytes --]
Hi,
This is a follow-up of PR64208 where an LRA loop was due to redundancy
in insn's alternatives. I've checked all the insns in the arm backend
to avoid latent problems and this patch fixes the issues I've spotted.
Only thumb2_movsicc_insn contained duplicated alternatives, and the
rest of the changes are related to the type attribute, which were not
accurate or used accordingly to their definition. Notice that the
modifications have only a limited impact as in most of the pipeline
descriptions, multiple/mov_reg/alu_reg/.. types are used the same way,
only cortex a8 seems to have a real difference between alu or multiple
instruction and mov.
Bootstrapped and regtested on arm-linux-gnueabihf. Ok for trunk ?
Thanks,
Yvan
2015-04-27 Yvan Roux <yvan.roux@linaro.org>
* config/arm/arm.mb (*arm_movt): Fix type attribute.
(*movsi_compare0): Likewise.
(*cmpsi_shiftsi): Likewise.
(*cmpsi_shiftsi_swp): Likewise.
(*movsicc_insn): Likewise.
(*cond_move): Likewise.
(*if_plus_move): Likewise.
(*if_move_plus): Likewise.
(*if_arith_move): Likewise.
(*if_move_arith): Likewise.
(*if_shift_move): Likewise.
(*if_move_shift): Likewise.
* config/arm/thumb2.md (*thumb2_movsi_insn): Fix type attribute.
(*thumb2_movsicc_insn): Fix alternative redundancy.
(*thumb2_addsi_short): Split register and immediate alternatives.
(thumb2_addsi3_compare0): Likewise.
[-- Attachment #2: insn.diff --]
[-- Type: text/plain, Size: 9975 bytes --]
diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 164ac13..ee23deb 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -5631,7 +5631,7 @@
[(set_attr "predicable" "yes")
(set_attr "predicable_short_it" "no")
(set_attr "length" "4")
- (set_attr "type" "mov_imm")]
+ (set_attr "type" "alu_sreg")]
)
(define_insn "*arm_movsi_insn"
@@ -5919,7 +5919,7 @@
cmp%?\\t%0, #0
sub%.\\t%0, %1, #0"
[(set_attr "conds" "set")
- (set_attr "type" "alus_imm,alus_imm")]
+ (set_attr "type" "alus_sreg,alus_sreg")]
)
;; Subroutine to store a half word from a register into memory.
@@ -6886,7 +6886,7 @@
[(set_attr "conds" "set")
(set_attr "shift" "1")
(set_attr "arch" "32,a,a")
- (set_attr "type" "alus_shift_imm,alu_shift_reg,alus_shift_imm")])
+ (set_attr "type" "alus_shift_imm,alus_shift_reg,alus_shift_imm")])
(define_insn "*cmpsi_shiftsi_swp"
[(set (reg:CC_SWP CC_REGNUM)
@@ -6899,7 +6899,7 @@
[(set_attr "conds" "set")
(set_attr "shift" "1")
(set_attr "arch" "32,a,a")
- (set_attr "type" "alus_shift_imm,alu_shift_reg,alus_shift_imm")])
+ (set_attr "type" "alus_shift_imm,alus_shift_reg,alus_shift_imm")])
(define_insn "*arm_cmpsi_negshiftsi_si"
[(set (reg:CC_Z CC_REGNUM)
@@ -7492,10 +7492,10 @@
(const_string "mov_imm")
(const_string "mov_reg"))
(const_string "mvn_imm")
- (const_string "mov_reg")
- (const_string "mov_reg")
- (const_string "mov_reg")
- (const_string "mov_reg")])]
+ (const_string "multiple")
+ (const_string "multiple")
+ (const_string "multiple")
+ (const_string "multiple")])]
)
(define_insn "*movsfcc_soft_insn"
@@ -8653,7 +8653,14 @@
return \"\";
"
[(set_attr "conds" "use")
- (set_attr "type" "mov_reg,mov_reg,multiple")
+ (set_attr_alternative "type"
+ [(if_then_else (match_operand 2 "const_int_operand" "")
+ (const_string "mov_imm")
+ (const_string "mov_reg"))
+ (if_then_else (match_operand 1 "const_int_operand" "")
+ (const_string "mov_imm")
+ (const_string "mov_reg"))
+ (const_string "multiple")])
(set_attr "length" "4,4,8")]
)
@@ -9449,8 +9456,8 @@
(const_string "alu_imm" )
(const_string "alu_sreg"))
(const_string "alu_imm")
- (const_string "alu_sreg")
- (const_string "alu_sreg")])]
+ (const_string "multiple")
+ (const_string "multiple")])]
)
(define_insn "*ifcompare_move_plus"
@@ -9487,7 +9494,13 @@
sub%D4\\t%0, %2, #%n3\;mov%d4\\t%0, %1"
[(set_attr "conds" "use")
(set_attr "length" "4,4,8,8")
- (set_attr "type" "alu_sreg,alu_imm,multiple,multiple")]
+ (set_attr_alternative "type"
+ [(if_then_else (match_operand 3 "const_int_operand" "")
+ (const_string "alu_imm" )
+ (const_string "alu_sreg"))
+ (const_string "alu_imm")
+ (const_string "multiple")
+ (const_string "multiple")])]
)
(define_insn "*ifcompare_arith_arith"
@@ -9582,7 +9595,11 @@
%I5%d4\\t%0, %2, %3\;mov%D4\\t%0, %1"
[(set_attr "conds" "use")
(set_attr "length" "4,8")
- (set_attr "type" "alu_shift_reg,multiple")]
+ (set_attr_alternative "type"
+ [(if_then_else (match_operand 3 "const_int_operand" "")
+ (const_string "alu_shift_imm" )
+ (const_string "alu_shift_reg"))
+ (const_string "multiple")])]
)
(define_insn "*ifcompare_move_arith"
@@ -9643,7 +9660,11 @@
%I5%D4\\t%0, %2, %3\;mov%d4\\t%0, %1"
[(set_attr "conds" "use")
(set_attr "length" "4,8")
- (set_attr "type" "alu_shift_reg,multiple")]
+ (set_attr_alternative "type"
+ [(if_then_else (match_operand 3 "const_int_operand" "")
+ (const_string "alu_shift_imm" )
+ (const_string "alu_shift_reg"))
+ (const_string "multiple")])]
)
(define_insn "*ifcompare_move_not"
@@ -9750,7 +9771,12 @@
[(set_attr "conds" "use")
(set_attr "shift" "2")
(set_attr "length" "4,8,8")
- (set_attr "type" "mov_shift_reg,multiple,multiple")]
+ (set_attr_alternative "type"
+ [(if_then_else (match_operand 3 "const_int_operand" "")
+ (const_string "mov_shift" )
+ (const_string "mov_shift_reg"))
+ (const_string "multiple")
+ (const_string "multiple")])]
)
(define_insn "*ifcompare_move_shift"
@@ -9788,7 +9814,12 @@
[(set_attr "conds" "use")
(set_attr "shift" "2")
(set_attr "length" "4,8,8")
- (set_attr "type" "mov_shift_reg,multiple,multiple")]
+ (set_attr_alternative "type"
+ [(if_then_else (match_operand 3 "const_int_operand" "")
+ (const_string "mov_shift" )
+ (const_string "mov_shift_reg"))
+ (const_string "multiple")
+ (const_string "multiple")])]
)
(define_insn "*ifcompare_shift_shift"
diff --git a/gcc/config/arm/thumb2.md b/gcc/config/arm/thumb2.md
index 1f68147..968116e 100644
--- a/gcc/config/arm/thumb2.md
+++ b/gcc/config/arm/thumb2.md
@@ -300,7 +300,7 @@
ldr%?\\t%0, %1
str%?\\t%1, %0
str%?\\t%1, %0"
- [(set_attr "type" "mov_reg,alu_imm,alu_imm,alu_imm,mov_imm,load1,load1,store1,store1")
+ [(set_attr "type" "mov_reg,mov_imm,mov_imm,mvn_imm,mov_imm,load1,load1,store1,store1")
(set_attr "length" "2,4,2,4,4,4,4,4,4")
(set_attr "predicable" "yes")
(set_attr "predicable_short_it" "yes,no,yes,no,no,no,no,no,no")
@@ -486,12 +486,12 @@
)
(define_insn_and_split "*thumb2_movsicc_insn"
- [(set (match_operand:SI 0 "s_register_operand" "=l,l,r,r,r,r,r,r,r,r,r")
+ [(set (match_operand:SI 0 "s_register_operand" "=l,l,r,r,r,r,r,r,r,r,r,r")
(if_then_else:SI
(match_operator 3 "arm_comparison_operator"
[(match_operand 4 "cc_register" "") (const_int 0)])
- (match_operand:SI 1 "arm_not_operand" "0 ,lPy,0 ,0,rI,K,rI,rI,K ,K,r")
- (match_operand:SI 2 "arm_not_operand" "lPy,0 ,rI,K,0 ,0,rI,K ,rI,K,r")))]
+ (match_operand:SI 1 "arm_not_operand" "0 ,lPy,0 ,0,rI,K,I ,r,rI,K ,K,r")
+ (match_operand:SI 2 "arm_not_operand" "lPy,0 ,rI,K,0 ,0,rI,I,K ,rI,K,r")))]
"TARGET_THUMB2"
"@
it\\t%D3\;mov%D3\\t%0, %2
@@ -504,12 +504,14 @@
#
#
#
+ #
#"
; alt 6: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
- ; alt 7: ite\\t%d3\;mov%d3\\t%0, %1\;mvn%D3\\t%0, #%B2
- ; alt 8: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mov%D3\\t%0, %2
- ; alt 9: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mvn%D3\\t%0, #%B2
- ; alt 10: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
+ ; alt 7: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
+ ; alt 8: ite\\t%d3\;mov%d3\\t%0, %1\;mvn%D3\\t%0, #%B2
+ ; alt 9: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mov%D3\\t%0, %2
+ ; alt 10: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mvn%D3\\t%0, #%B2
+ ; alt 11: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
"&& reload_completed"
[(const_int 0)]
{
@@ -540,8 +542,8 @@
operands[2])));
DONE;
}
- [(set_attr "length" "4,4,6,6,6,6,10,10,10,10,6")
- (set_attr "enabled_for_depr_it" "yes,yes,no,no,no,no,no,no,no,no,yes")
+ [(set_attr "length" "4,4,6,6,6,6,10,8,10,10,10,6")
+ (set_attr "enabled_for_depr_it" "yes,yes,no,no,no,no,no,no,no,no,no,yes")
(set_attr "conds" "use")
(set_attr "type" "multiple")]
)
@@ -1161,9 +1163,9 @@
)
(define_insn "*thumb2_addsi_short"
- [(set (match_operand:SI 0 "low_register_operand" "=l,l")
- (plus:SI (match_operand:SI 1 "low_register_operand" "l,0")
- (match_operand:SI 2 "low_reg_or_int_operand" "lPt,Ps")))
+ [(set (match_operand:SI 0 "low_register_operand" "=l,l,l")
+ (plus:SI (match_operand:SI 1 "low_register_operand" "l,l,0")
+ (match_operand:SI 2 "low_reg_or_int_operand" "l,Pt,Ps")))
(clobber (reg:CC CC_REGNUM))]
"TARGET_THUMB2 && reload_completed"
"*
@@ -1182,7 +1184,7 @@
"
[(set_attr "predicable" "yes")
(set_attr "length" "2")
- (set_attr "type" "alu_sreg")]
+ (set_attr "type" "alu_sreg,alu_imm,alu_imm")]
)
(define_insn "*thumb2_subsi_short"
@@ -1226,10 +1228,10 @@
(define_insn "thumb2_addsi3_compare0"
[(set (reg:CC_NOOV CC_REGNUM)
(compare:CC_NOOV
- (plus:SI (match_operand:SI 1 "s_register_operand" "l, 0, r")
- (match_operand:SI 2 "arm_add_operand" "lPt,Ps,rIL"))
+ (plus:SI (match_operand:SI 1 "s_register_operand" "l,l, 0,r,r")
+ (match_operand:SI 2 "arm_add_operand" "l,Pt,Ps,r,IL"))
(const_int 0)))
- (set (match_operand:SI 0 "s_register_operand" "=l,l,r")
+ (set (match_operand:SI 0 "s_register_operand" "=l,l,l,r,r")
(plus:SI (match_dup 1) (match_dup 2)))]
"TARGET_THUMB2"
"*
@@ -1246,8 +1248,8 @@
return \"adds\\t%0, %1, %2\";
"
[(set_attr "conds" "set")
- (set_attr "length" "2,2,4")
- (set_attr "type" "alu_sreg")]
+ (set_attr "length" "2,2,2,4,4")
+ (set_attr "type" "alus_sreg,alus_imm,alus_imm,alus_sreg,alus_imm")]
)
(define_insn "*thumb2_addsi3_compare0_scratch"
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH, ARM] Alternatives and type attributes fixes.
2015-04-27 12:25 [PATCH, ARM] Alternatives and type attributes fixes Yvan Roux
@ 2015-04-27 13:20 ` Kyrill Tkachov
2015-04-27 13:58 ` Yvan Roux
0 siblings, 1 reply; 5+ messages in thread
From: Kyrill Tkachov @ 2015-04-27 13:20 UTC (permalink / raw)
To: Yvan Roux, gcc-patches, Ramana Radhakrishnan, Richard Earnshaw
Hi Yvan,
On 27/04/15 13:25, Yvan Roux wrote:
> Hi,
>
> This is a follow-up of PR64208 where an LRA loop was due to redundancy
> in insn's alternatives. I've checked all the insns in the arm backend
> to avoid latent problems and this patch fixes the issues I've spotted.
>
> Only thumb2_movsicc_insn contained duplicated alternatives, and the
> rest of the changes are related to the type attribute, which were not
> accurate or used accordingly to their definition. Notice that the
> modifications have only a limited impact as in most of the pipeline
> descriptions, multiple/mov_reg/alu_reg/.. types are used the same way,
> only cortex a8 seems to have a real difference between alu or multiple
> instruction and mov.
>
> Bootstrapped and regtested on arm-linux-gnueabihf. Ok for trunk ?
>
> Thanks,
> Yvan
>
> 2015-04-27 Yvan Roux<yvan.roux@linaro.org>
>
> * config/arm/arm.mb (*arm_movt): Fix type attribute.
> (*movsi_compare0): Likewise.
> (*cmpsi_shiftsi): Likewise.
> (*cmpsi_shiftsi_swp): Likewise.
> (*movsicc_insn): Likewise.
> (*cond_move): Likewise.
> (*if_plus_move): Likewise.
> (*if_move_plus): Likewise.
> (*if_arith_move): Likewise.
> (*if_move_arith): Likewise.
> (*if_shift_move): Likewise.
> (*if_move_shift): Likewise.
> * config/arm/thumb2.md (*thumb2_movsi_insn): Fix type attribute.
> (*thumb2_movsicc_insn): Fix alternative redundancy.
> (*thumb2_addsi_short): Split register and immediate alternatives.
> (thumb2_addsi3_compare0): Likewise.
>
> insn.diff
>
>
> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
> index 164ac13..ee23deb 100644
> --- a/gcc/config/arm/arm.md
> +++ b/gcc/config/arm/arm.md
> @@ -5631,7 +5631,7 @@
> [(set_attr "predicable" "yes")
> (set_attr "predicable_short_it" "no")
> (set_attr "length" "4")
> - (set_attr "type" "mov_imm")]
> + (set_attr "type" "alu_sreg")]
> )
For some context, this is the *arm_movt pattern that generates
a movt instruction. The documentation in types.md says:
"This excludes MOV and MVN but includes MOVT." So using alu_sreg
is correct according to that logic.
However, don't you want to also update the arm_movtas_ze pattern
that generates movt but erroneously has the type 'mov_imm'?
>
> (define_insn "*arm_movsi_insn"
> @@ -5919,7 +5919,7 @@
> cmp%?\\t%0, #0
> sub%.\\t%0, %1, #0"
> [(set_attr "conds" "set")
> - (set_attr "type" "alus_imm,alus_imm")]
> + (set_attr "type" "alus_sreg,alus_sreg")]
> )
Why change that? They are instructions with immediate operands
so alus_imm should be gorrect?
> @@ -486,12 +486,12 @@
> )
>
> (define_insn_and_split "*thumb2_movsicc_insn"
> - [(set (match_operand:SI 0 "s_register_operand" "=l,l,r,r,r,r,r,r,r,r,r")
> + [(set (match_operand:SI 0 "s_register_operand" "=l,l,r,r,r,r,r,r,r,r,r,r")
> (if_then_else:SI
> (match_operator 3 "arm_comparison_operator"
> [(match_operand 4 "cc_register" "") (const_int 0)])
> - (match_operand:SI 1 "arm_not_operand" "0 ,lPy,0 ,0,rI,K,rI,rI,K ,K,r")
> - (match_operand:SI 2 "arm_not_operand" "lPy,0 ,rI,K,0 ,0,rI,K ,rI,K,r")))]
> + (match_operand:SI 1 "arm_not_operand" "0 ,lPy,0 ,0,rI,K,I ,r,rI,K ,K,r")
> + (match_operand:SI 2 "arm_not_operand" "lPy,0 ,rI,K,0 ,0,rI,I,K ,rI,K,r")))]
> "TARGET_THUMB2"
> "@
> it\\t%D3\;mov%D3\\t%0, %2
> @@ -504,12 +504,14 @@
> #
> #
> #
> + #
> #"
> ; alt 6: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
> - ; alt 7: ite\\t%d3\;mov%d3\\t%0, %1\;mvn%D3\\t%0, #%B2
> - ; alt 8: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mov%D3\\t%0, %2
> - ; alt 9: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mvn%D3\\t%0, #%B2
> - ; alt 10: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
> + ; alt 7: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
> + ; alt 8: ite\\t%d3\;mov%d3\\t%0, %1\;mvn%D3\\t%0, #%B2
> + ; alt 9: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mov%D3\\t%0, %2
> + ; alt 10: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mvn%D3\\t%0, #%B2
> + ; alt 11: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
> "&& reload_completed"
> [(const_int 0)]
> {
> @@ -540,8 +542,8 @@
> operands[2])));
> DONE;
> }
> - [(set_attr "length" "4,4,6,6,6,6,10,10,10,10,6")
> - (set_attr "enabled_for_depr_it" "yes,yes,no,no,no,no,no,no,no,no,yes")
> + [(set_attr "length" "4,4,6,6,6,6,10,8,10,10,10,6")
> + (set_attr "enabled_for_depr_it" "yes,yes,no,no,no,no,no,no,no,no,no,yes")
> (set_attr "conds" "use")
> (set_attr "type" "multiple")]
> )
So, I think here for the first 6 alternatives we can give it a more specific type,
like mov_immm or mov_reg, since you're cleaning this stuff up. The instructions in
those alternatives are just a mov instruction enclosed in an IT block, so they always
have to stick together anyway.
> @@ -1161,9 +1163,9 @@
> )
>
> (define_insn "*thumb2_addsi_short"
> - [(set (match_operand:SI 0 "low_register_operand" "=l,l")
> - (plus:SI (match_operand:SI 1 "low_register_operand" "l,0")
> - (match_operand:SI 2 "low_reg_or_int_operand" "lPt,Ps")))
> + [(set (match_operand:SI 0 "low_register_operand" "=l,l,l")
> + (plus:SI (match_operand:SI 1 "low_register_operand" "l,l,0")
> + (match_operand:SI 2 "low_reg_or_int_operand" "l,Pt,Ps")))
> (clobber (reg:CC CC_REGNUM))]
> "TARGET_THUMB2 && reload_completed"
> "*
> @@ -1182,7 +1184,7 @@
> "
> [(set_attr "predicable" "yes")
> (set_attr "length" "2")
> - (set_attr "type" "alu_sreg")]
> + (set_attr "type" "alu_sreg,alu_imm,alu_imm")]
> )
>
> (define_insn "*thumb2_subsi_short"
> @@ -1226,10 +1228,10 @@
> (define_insn "thumb2_addsi3_compare0"
> [(set (reg:CC_NOOV CC_REGNUM)
> (compare:CC_NOOV
> - (plus:SI (match_operand:SI 1 "s_register_operand" "l, 0, r")
> - (match_operand:SI 2 "arm_add_operand" "lPt,Ps,rIL"))
> + (plus:SI (match_operand:SI 1 "s_register_operand" "l,l, 0,r,r")
> + (match_operand:SI 2 "arm_add_operand" "l,Pt,Ps,r,IL"))
> (const_int 0)))
> - (set (match_operand:SI 0 "s_register_operand" "=l,l,r")
> + (set (match_operand:SI 0 "s_register_operand" "=l,l,l,r,r")
> (plus:SI (match_dup 1) (match_dup 2)))]
> "TARGET_THUMB2"
> "*
> @@ -1246,8 +1248,8 @@
> return \"adds\\t%0, %1, %2\";
> "
> [(set_attr "conds" "set")
> - (set_attr "length" "2,2,4")
> - (set_attr "type" "alu_sreg")]
> + (set_attr "length" "2,2,2,4,4")
> + (set_attr "type" "alus_sreg,alus_imm,alus_imm,alus_sreg,alus_imm")]
> )
>
In the other patterns in arm.md you didn't split up the alternatives
but instead used an if_then_else in the set_attr_alternative to differentiate
the case where the operand is constant.
Any particular reason why you split up the alternatives here?
In my opinion, having fewer alternatives at the expense of a more complex definition
of 'type' is preferable, but someone might have a stronger opinion in the other
direction.
The patch looks ok to me otherwise, but please respin with the comments above addressed.
Thanks for cleaning this up!
Kyrill
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH, ARM] Alternatives and type attributes fixes.
2015-04-27 13:20 ` Kyrill Tkachov
@ 2015-04-27 13:58 ` Yvan Roux
2015-04-28 8:55 ` Yvan Roux
0 siblings, 1 reply; 5+ messages in thread
From: Yvan Roux @ 2015-04-27 13:58 UTC (permalink / raw)
To: Kyrill Tkachov; +Cc: gcc-patches, Ramana Radhakrishnan, Richard Earnshaw
On 27 April 2015 at 15:20, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
> Hi Yvan,
>
>
> On 27/04/15 13:25, Yvan Roux wrote:
>>
>> Hi,
>>
>> This is a follow-up of PR64208 where an LRA loop was due to redundancy
>> in insn's alternatives. I've checked all the insns in the arm backend
>> to avoid latent problems and this patch fixes the issues I've spotted.
>>
>> Only thumb2_movsicc_insn contained duplicated alternatives, and the
>> rest of the changes are related to the type attribute, which were not
>> accurate or used accordingly to their definition. Notice that the
>> modifications have only a limited impact as in most of the pipeline
>> descriptions, multiple/mov_reg/alu_reg/.. types are used the same way,
>> only cortex a8 seems to have a real difference between alu or multiple
>> instruction and mov.
>>
>> Bootstrapped and regtested on arm-linux-gnueabihf. Ok for trunk ?
>>
>> Thanks,
>> Yvan
>>
>> 2015-04-27 Yvan Roux<yvan.roux@linaro.org>
>>
>> * config/arm/arm.mb (*arm_movt): Fix type attribute.
>> (*movsi_compare0): Likewise.
>> (*cmpsi_shiftsi): Likewise.
>> (*cmpsi_shiftsi_swp): Likewise.
>> (*movsicc_insn): Likewise.
>> (*cond_move): Likewise.
>> (*if_plus_move): Likewise.
>> (*if_move_plus): Likewise.
>> (*if_arith_move): Likewise.
>> (*if_move_arith): Likewise.
>> (*if_shift_move): Likewise.
>> (*if_move_shift): Likewise.
>> * config/arm/thumb2.md (*thumb2_movsi_insn): Fix type attribute.
>> (*thumb2_movsicc_insn): Fix alternative redundancy.
>> (*thumb2_addsi_short): Split register and immediate alternatives.
>> (thumb2_addsi3_compare0): Likewise.
>>
>> insn.diff
>>
>>
>> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
>> index 164ac13..ee23deb 100644
>> --- a/gcc/config/arm/arm.md
>> +++ b/gcc/config/arm/arm.md
>> @@ -5631,7 +5631,7 @@
>> [(set_attr "predicable" "yes")
>> (set_attr "predicable_short_it" "no")
>> (set_attr "length" "4")
>> - (set_attr "type" "mov_imm")]
>> + (set_attr "type" "alu_sreg")]
>> )
>
>
> For some context, this is the *arm_movt pattern that generates
> a movt instruction. The documentation in types.md says:
> "This excludes MOV and MVN but includes MOVT." So using alu_sreg
> is correct according to that logic.
> However, don't you want to also update the arm_movtas_ze pattern
> that generates movt but erroneously has the type 'mov_imm'?
Ha, yes I missed this one ! I'll will update it.
>> (define_insn "*arm_movsi_insn"
>> @@ -5919,7 +5919,7 @@
>> cmp%?\\t%0, #0
>> sub%.\\t%0, %1, #0"
>> [(set_attr "conds" "set")
>> - (set_attr "type" "alus_imm,alus_imm")]
>> + (set_attr "type" "alus_sreg,alus_sreg")]
>> )
>
>
> Why change that? They are instructions with immediate operands
> so alus_imm should be gorrect?
Oups, I certainly misread #0 and %0 this one is correct.
>> @@ -486,12 +486,12 @@
>> )
>> (define_insn_and_split "*thumb2_movsicc_insn"
>> - [(set (match_operand:SI 0 "s_register_operand"
>> "=l,l,r,r,r,r,r,r,r,r,r")
>> + [(set (match_operand:SI 0 "s_register_operand"
>> "=l,l,r,r,r,r,r,r,r,r,r,r")
>> (if_then_else:SI
>> (match_operator 3 "arm_comparison_operator"
>> [(match_operand 4 "cc_register" "") (const_int 0)])
>> - (match_operand:SI 1 "arm_not_operand" "0 ,lPy,0 ,0,rI,K,rI,rI,K
>> ,K,r")
>> - (match_operand:SI 2 "arm_not_operand" "lPy,0 ,rI,K,0 ,0,rI,K
>> ,rI,K,r")))]
>> + (match_operand:SI 1 "arm_not_operand" "0 ,lPy,0 ,0,rI,K,I ,r,rI,K
>> ,K,r")
>> + (match_operand:SI 2 "arm_not_operand" "lPy,0 ,rI,K,0 ,0,rI,I,K
>> ,rI,K,r")))]
>> "TARGET_THUMB2"
>> "@
>> it\\t%D3\;mov%D3\\t%0, %2
>> @@ -504,12 +504,14 @@
>> #
>> #
>> #
>> + #
>> #"
>> ; alt 6: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
>> - ; alt 7: ite\\t%d3\;mov%d3\\t%0, %1\;mvn%D3\\t%0, #%B2
>> - ; alt 8: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mov%D3\\t%0, %2
>> - ; alt 9: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mvn%D3\\t%0, #%B2
>> - ; alt 10: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
>> + ; alt 7: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
>> + ; alt 8: ite\\t%d3\;mov%d3\\t%0, %1\;mvn%D3\\t%0, #%B2
>> + ; alt 9: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mov%D3\\t%0, %2
>> + ; alt 10: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mvn%D3\\t%0, #%B2
>> + ; alt 11: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
>> "&& reload_completed"
>> [(const_int 0)]
>> {
>> @@ -540,8 +542,8 @@
>> operands[2])));
>> DONE;
>> }
>> - [(set_attr "length" "4,4,6,6,6,6,10,10,10,10,6")
>> - (set_attr "enabled_for_depr_it" "yes,yes,no,no,no,no,no,no,no,no,yes")
>> + [(set_attr "length" "4,4,6,6,6,6,10,8,10,10,10,6")
>> + (set_attr "enabled_for_depr_it"
>> "yes,yes,no,no,no,no,no,no,no,no,no,yes")
>> (set_attr "conds" "use")
>> (set_attr "type" "multiple")]
>> )
>
>
> So, I think here for the first 6 alternatives we can give it a more specific
> type,
> like mov_immm or mov_reg, since you're cleaning this stuff up. The
> instructions in
> those alternatives are just a mov instruction enclosed in an IT block, so
> they always
> have to stick together anyway.
OK I'll change that.
>> @@ -1161,9 +1163,9 @@
>> )
>> (define_insn "*thumb2_addsi_short"
>> - [(set (match_operand:SI 0 "low_register_operand" "=l,l")
>> - (plus:SI (match_operand:SI 1 "low_register_operand" "l,0")
>> - (match_operand:SI 2 "low_reg_or_int_operand" "lPt,Ps")))
>> + [(set (match_operand:SI 0 "low_register_operand" "=l,l,l")
>> + (plus:SI (match_operand:SI 1 "low_register_operand" "l,l,0")
>> + (match_operand:SI 2 "low_reg_or_int_operand" "l,Pt,Ps")))
>> (clobber (reg:CC CC_REGNUM))]
>> "TARGET_THUMB2 && reload_completed"
>> "*
>> @@ -1182,7 +1184,7 @@
>> "
>> [(set_attr "predicable" "yes")
>> (set_attr "length" "2")
>> - (set_attr "type" "alu_sreg")]
>> + (set_attr "type" "alu_sreg,alu_imm,alu_imm")]
>> )
>> (define_insn "*thumb2_subsi_short"
>> @@ -1226,10 +1228,10 @@
>> (define_insn "thumb2_addsi3_compare0"
>> [(set (reg:CC_NOOV CC_REGNUM)
>> (compare:CC_NOOV
>> - (plus:SI (match_operand:SI 1 "s_register_operand" "l, 0, r")
>> - (match_operand:SI 2 "arm_add_operand" "lPt,Ps,rIL"))
>> + (plus:SI (match_operand:SI 1 "s_register_operand" "l,l, 0,r,r")
>> + (match_operand:SI 2 "arm_add_operand"
>> "l,Pt,Ps,r,IL"))
>> (const_int 0)))
>> - (set (match_operand:SI 0 "s_register_operand" "=l,l,r")
>> + (set (match_operand:SI 0 "s_register_operand" "=l,l,l,r,r")
>> (plus:SI (match_dup 1) (match_dup 2)))]
>> "TARGET_THUMB2"
>> "*
>> @@ -1246,8 +1248,8 @@
>> return \"adds\\t%0, %1, %2\";
>> "
>> [(set_attr "conds" "set")
>> - (set_attr "length" "2,2,4")
>> - (set_attr "type" "alu_sreg")]
>> + (set_attr "length" "2,2,2,4,4")
>> + (set_attr "type" "alus_sreg,alus_imm,alus_imm,alus_sreg,alus_imm")]
>> )
>>
>
>
> In the other patterns in arm.md you didn't split up the alternatives
> but instead used an if_then_else in the set_attr_alternative to
> differentiate
> the case where the operand is constant.
> Any particular reason why you split up the alternatives here?
I think that I tried to be consistent with the implementation of
*thumb2_addsi3_compare0_scratch insn, it is also due to this work was
interrupted several time and I wasn't really consistent with myself ;)
> In my opinion, having fewer alternatives at the expense of a more complex
> definition
> of 'type' is preferable, but someone might have a stronger opinion in the
> other
> direction.
I also prefer fewer alternatives, I'll rework the patch that way and
refactor *thumb2_addsi3_compare0_scratch too (or will do the opposite
if a good argument comes in that thread).
> The patch looks ok to me otherwise, but please respin with the comments
> above addressed.
>
> Thanks for cleaning this up!
Thanks for the review Kyrill
Cheers,
Yvan
> Kyrill
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH, ARM] Alternatives and type attributes fixes.
2015-04-27 13:58 ` Yvan Roux
@ 2015-04-28 8:55 ` Yvan Roux
2015-04-28 14:47 ` Kyrill Tkachov
0 siblings, 1 reply; 5+ messages in thread
From: Yvan Roux @ 2015-04-28 8:55 UTC (permalink / raw)
To: Kyrill Tkachov; +Cc: gcc-patches, Ramana Radhakrishnan, Richard Earnshaw
[-- Attachment #1: Type: text/plain, Size: 9212 bytes --]
Hi,
On 27 April 2015 at 15:58, Yvan Roux <yvan.roux@linaro.org> wrote:
> On 27 April 2015 at 15:20, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
>> Hi Yvan,
>>
>>
>> On 27/04/15 13:25, Yvan Roux wrote:
>>>
>>> Hi,
>>>
>>> This is a follow-up of PR64208 where an LRA loop was due to redundancy
>>> in insn's alternatives. I've checked all the insns in the arm backend
>>> to avoid latent problems and this patch fixes the issues I've spotted.
>>>
>>> Only thumb2_movsicc_insn contained duplicated alternatives, and the
>>> rest of the changes are related to the type attribute, which were not
>>> accurate or used accordingly to their definition. Notice that the
>>> modifications have only a limited impact as in most of the pipeline
>>> descriptions, multiple/mov_reg/alu_reg/.. types are used the same way,
>>> only cortex a8 seems to have a real difference between alu or multiple
>>> instruction and mov.
>>>
>>> Bootstrapped and regtested on arm-linux-gnueabihf. Ok for trunk ?
>>>
>>> Thanks,
>>> Yvan
>>>
>>> 2015-04-27 Yvan Roux<yvan.roux@linaro.org>
>>>
>>> * config/arm/arm.mb (*arm_movt): Fix type attribute.
>>> (*movsi_compare0): Likewise.
>>> (*cmpsi_shiftsi): Likewise.
>>> (*cmpsi_shiftsi_swp): Likewise.
>>> (*movsicc_insn): Likewise.
>>> (*cond_move): Likewise.
>>> (*if_plus_move): Likewise.
>>> (*if_move_plus): Likewise.
>>> (*if_arith_move): Likewise.
>>> (*if_move_arith): Likewise.
>>> (*if_shift_move): Likewise.
>>> (*if_move_shift): Likewise.
>>> * config/arm/thumb2.md (*thumb2_movsi_insn): Fix type attribute.
>>> (*thumb2_movsicc_insn): Fix alternative redundancy.
>>> (*thumb2_addsi_short): Split register and immediate alternatives.
>>> (thumb2_addsi3_compare0): Likewise.
>>>
>>> insn.diff
>>>
>>>
>>> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
>>> index 164ac13..ee23deb 100644
>>> --- a/gcc/config/arm/arm.md
>>> +++ b/gcc/config/arm/arm.md
>>> @@ -5631,7 +5631,7 @@
>>> [(set_attr "predicable" "yes")
>>> (set_attr "predicable_short_it" "no")
>>> (set_attr "length" "4")
>>> - (set_attr "type" "mov_imm")]
>>> + (set_attr "type" "alu_sreg")]
>>> )
>>
>>
>> For some context, this is the *arm_movt pattern that generates
>> a movt instruction. The documentation in types.md says:
>> "This excludes MOV and MVN but includes MOVT." So using alu_sreg
>> is correct according to that logic.
>> However, don't you want to also update the arm_movtas_ze pattern
>> that generates movt but erroneously has the type 'mov_imm'?
>
> Ha, yes I missed this one ! I'll will update it.
>
>>> (define_insn "*arm_movsi_insn"
>>> @@ -5919,7 +5919,7 @@
>>> cmp%?\\t%0, #0
>>> sub%.\\t%0, %1, #0"
>>> [(set_attr "conds" "set")
>>> - (set_attr "type" "alus_imm,alus_imm")]
>>> + (set_attr "type" "alus_sreg,alus_sreg")]
>>> )
>>
>>
>> Why change that? They are instructions with immediate operands
>> so alus_imm should be gorrect?
>
> Oups, I certainly misread #0 and %0 this one is correct.
>
>>> @@ -486,12 +486,12 @@
>>> )
>>> (define_insn_and_split "*thumb2_movsicc_insn"
>>> - [(set (match_operand:SI 0 "s_register_operand"
>>> "=l,l,r,r,r,r,r,r,r,r,r")
>>> + [(set (match_operand:SI 0 "s_register_operand"
>>> "=l,l,r,r,r,r,r,r,r,r,r,r")
>>> (if_then_else:SI
>>> (match_operator 3 "arm_comparison_operator"
>>> [(match_operand 4 "cc_register" "") (const_int 0)])
>>> - (match_operand:SI 1 "arm_not_operand" "0 ,lPy,0 ,0,rI,K,rI,rI,K
>>> ,K,r")
>>> - (match_operand:SI 2 "arm_not_operand" "lPy,0 ,rI,K,0 ,0,rI,K
>>> ,rI,K,r")))]
>>> + (match_operand:SI 1 "arm_not_operand" "0 ,lPy,0 ,0,rI,K,I ,r,rI,K
>>> ,K,r")
>>> + (match_operand:SI 2 "arm_not_operand" "lPy,0 ,rI,K,0 ,0,rI,I,K
>>> ,rI,K,r")))]
>>> "TARGET_THUMB2"
>>> "@
>>> it\\t%D3\;mov%D3\\t%0, %2
>>> @@ -504,12 +504,14 @@
>>> #
>>> #
>>> #
>>> + #
>>> #"
>>> ; alt 6: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
>>> - ; alt 7: ite\\t%d3\;mov%d3\\t%0, %1\;mvn%D3\\t%0, #%B2
>>> - ; alt 8: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mov%D3\\t%0, %2
>>> - ; alt 9: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mvn%D3\\t%0, #%B2
>>> - ; alt 10: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
>>> + ; alt 7: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
>>> + ; alt 8: ite\\t%d3\;mov%d3\\t%0, %1\;mvn%D3\\t%0, #%B2
>>> + ; alt 9: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mov%D3\\t%0, %2
>>> + ; alt 10: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mvn%D3\\t%0, #%B2
>>> + ; alt 11: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
>>> "&& reload_completed"
>>> [(const_int 0)]
>>> {
>>> @@ -540,8 +542,8 @@
>>> operands[2])));
>>> DONE;
>>> }
>>> - [(set_attr "length" "4,4,6,6,6,6,10,10,10,10,6")
>>> - (set_attr "enabled_for_depr_it" "yes,yes,no,no,no,no,no,no,no,no,yes")
>>> + [(set_attr "length" "4,4,6,6,6,6,10,8,10,10,10,6")
>>> + (set_attr "enabled_for_depr_it"
>>> "yes,yes,no,no,no,no,no,no,no,no,no,yes")
>>> (set_attr "conds" "use")
>>> (set_attr "type" "multiple")]
>>> )
>>
>>
>> So, I think here for the first 6 alternatives we can give it a more specific
>> type,
>> like mov_immm or mov_reg, since you're cleaning this stuff up. The
>> instructions in
>> those alternatives are just a mov instruction enclosed in an IT block, so
>> they always
>> have to stick together anyway.
>
> OK I'll change that.
>
>>> @@ -1161,9 +1163,9 @@
>>> )
>>> (define_insn "*thumb2_addsi_short"
>>> - [(set (match_operand:SI 0 "low_register_operand" "=l,l")
>>> - (plus:SI (match_operand:SI 1 "low_register_operand" "l,0")
>>> - (match_operand:SI 2 "low_reg_or_int_operand" "lPt,Ps")))
>>> + [(set (match_operand:SI 0 "low_register_operand" "=l,l,l")
>>> + (plus:SI (match_operand:SI 1 "low_register_operand" "l,l,0")
>>> + (match_operand:SI 2 "low_reg_or_int_operand" "l,Pt,Ps")))
>>> (clobber (reg:CC CC_REGNUM))]
>>> "TARGET_THUMB2 && reload_completed"
>>> "*
>>> @@ -1182,7 +1184,7 @@
>>> "
>>> [(set_attr "predicable" "yes")
>>> (set_attr "length" "2")
>>> - (set_attr "type" "alu_sreg")]
>>> + (set_attr "type" "alu_sreg,alu_imm,alu_imm")]
>>> )
>>> (define_insn "*thumb2_subsi_short"
>>> @@ -1226,10 +1228,10 @@
>>> (define_insn "thumb2_addsi3_compare0"
>>> [(set (reg:CC_NOOV CC_REGNUM)
>>> (compare:CC_NOOV
>>> - (plus:SI (match_operand:SI 1 "s_register_operand" "l, 0, r")
>>> - (match_operand:SI 2 "arm_add_operand" "lPt,Ps,rIL"))
>>> + (plus:SI (match_operand:SI 1 "s_register_operand" "l,l, 0,r,r")
>>> + (match_operand:SI 2 "arm_add_operand"
>>> "l,Pt,Ps,r,IL"))
>>> (const_int 0)))
>>> - (set (match_operand:SI 0 "s_register_operand" "=l,l,r")
>>> + (set (match_operand:SI 0 "s_register_operand" "=l,l,l,r,r")
>>> (plus:SI (match_dup 1) (match_dup 2)))]
>>> "TARGET_THUMB2"
>>> "*
>>> @@ -1246,8 +1248,8 @@
>>> return \"adds\\t%0, %1, %2\";
>>> "
>>> [(set_attr "conds" "set")
>>> - (set_attr "length" "2,2,4")
>>> - (set_attr "type" "alu_sreg")]
>>> + (set_attr "length" "2,2,2,4,4")
>>> + (set_attr "type" "alus_sreg,alus_imm,alus_imm,alus_sreg,alus_imm")]
>>> )
>>>
>>
>>
>> In the other patterns in arm.md you didn't split up the alternatives
>> but instead used an if_then_else in the set_attr_alternative to
>> differentiate
>> the case where the operand is constant.
>> Any particular reason why you split up the alternatives here?
>
> I think that I tried to be consistent with the implementation of
> *thumb2_addsi3_compare0_scratch insn, it is also due to this work was
> interrupted several time and I wasn't really consistent with myself ;)
>
>> In my opinion, having fewer alternatives at the expense of a more complex
>> definition
>> of 'type' is preferable, but someone might have a stronger opinion in the
>> other
>> direction.
>
> I also prefer fewer alternatives, I'll rework the patch that way and
> refactor *thumb2_addsi3_compare0_scratch too (or will do the opposite
> if a good argument comes in that thread).
>
>> The patch looks ok to me otherwise, but please respin with the comments
>> above addressed.
Here is the new patch with the needed modifications. Rebuilt and
regtested on arm-linux-gnueabihf.
Cheers,
Yvan
2015-04-28 Yvan Roux <yvan.roux@linaro.org>
* config/arm/arm.mb (*arm_movt): Fix type attribute.
(*cmpsi_shiftsi): Likewise.
(*cmpsi_shiftsi_swp): Likewise.
(*movsicc_insn): Likewise.
(*cond_move): Likewise.
(*if_plus_move): Likewise.
(*if_move_plus): Likewise.
(*if_arith_move): Likewise.
(*if_move_arith): Likewise.
(*if_shift_move): Likewise.
(*if_move_shift): Likewise.
(*arm_movtas_ze): Likewise.
* config/arm/thumb2.md (*thumb2_movsicc_insn): Fix alternative
redundancy and
type attributes.
(*thumb2_movsi_insn): Fix type attribute.
(*thumb2_addsi_short): Likewise.
(thumb2_addsi3_compare0): Likewise.
(*thumb2_addsi3_compare0_scratch): Merge alternatives and fix attributes
accordingly.
[-- Attachment #2: insn2.diff --]
[-- Type: text/plain, Size: 11659 bytes --]
diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 164ac13..a63ec96 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -5631,7 +5631,7 @@
[(set_attr "predicable" "yes")
(set_attr "predicable_short_it" "no")
(set_attr "length" "4")
- (set_attr "type" "mov_imm")]
+ (set_attr "type" "alu_sreg")]
)
(define_insn "*arm_movsi_insn"
@@ -6886,7 +6886,7 @@
[(set_attr "conds" "set")
(set_attr "shift" "1")
(set_attr "arch" "32,a,a")
- (set_attr "type" "alus_shift_imm,alu_shift_reg,alus_shift_imm")])
+ (set_attr "type" "alus_shift_imm,alus_shift_reg,alus_shift_imm")])
(define_insn "*cmpsi_shiftsi_swp"
[(set (reg:CC_SWP CC_REGNUM)
@@ -6899,7 +6899,7 @@
[(set_attr "conds" "set")
(set_attr "shift" "1")
(set_attr "arch" "32,a,a")
- (set_attr "type" "alus_shift_imm,alu_shift_reg,alus_shift_imm")])
+ (set_attr "type" "alus_shift_imm,alus_shift_reg,alus_shift_imm")])
(define_insn "*arm_cmpsi_negshiftsi_si"
[(set (reg:CC_Z CC_REGNUM)
@@ -7492,10 +7492,10 @@
(const_string "mov_imm")
(const_string "mov_reg"))
(const_string "mvn_imm")
- (const_string "mov_reg")
- (const_string "mov_reg")
- (const_string "mov_reg")
- (const_string "mov_reg")])]
+ (const_string "multiple")
+ (const_string "multiple")
+ (const_string "multiple")
+ (const_string "multiple")])]
)
(define_insn "*movsfcc_soft_insn"
@@ -8653,7 +8653,14 @@
return \"\";
"
[(set_attr "conds" "use")
- (set_attr "type" "mov_reg,mov_reg,multiple")
+ (set_attr_alternative "type"
+ [(if_then_else (match_operand 2 "const_int_operand" "")
+ (const_string "mov_imm")
+ (const_string "mov_reg"))
+ (if_then_else (match_operand 1 "const_int_operand" "")
+ (const_string "mov_imm")
+ (const_string "mov_reg"))
+ (const_string "multiple")])
(set_attr "length" "4,4,8")]
)
@@ -9449,8 +9456,8 @@
(const_string "alu_imm" )
(const_string "alu_sreg"))
(const_string "alu_imm")
- (const_string "alu_sreg")
- (const_string "alu_sreg")])]
+ (const_string "multiple")
+ (const_string "multiple")])]
)
(define_insn "*ifcompare_move_plus"
@@ -9487,7 +9494,13 @@
sub%D4\\t%0, %2, #%n3\;mov%d4\\t%0, %1"
[(set_attr "conds" "use")
(set_attr "length" "4,4,8,8")
- (set_attr "type" "alu_sreg,alu_imm,multiple,multiple")]
+ (set_attr_alternative "type"
+ [(if_then_else (match_operand 3 "const_int_operand" "")
+ (const_string "alu_imm" )
+ (const_string "alu_sreg"))
+ (const_string "alu_imm")
+ (const_string "multiple")
+ (const_string "multiple")])]
)
(define_insn "*ifcompare_arith_arith"
@@ -9582,7 +9595,11 @@
%I5%d4\\t%0, %2, %3\;mov%D4\\t%0, %1"
[(set_attr "conds" "use")
(set_attr "length" "4,8")
- (set_attr "type" "alu_shift_reg,multiple")]
+ (set_attr_alternative "type"
+ [(if_then_else (match_operand 3 "const_int_operand" "")
+ (const_string "alu_shift_imm" )
+ (const_string "alu_shift_reg"))
+ (const_string "multiple")])]
)
(define_insn "*ifcompare_move_arith"
@@ -9643,7 +9660,11 @@
%I5%D4\\t%0, %2, %3\;mov%d4\\t%0, %1"
[(set_attr "conds" "use")
(set_attr "length" "4,8")
- (set_attr "type" "alu_shift_reg,multiple")]
+ (set_attr_alternative "type"
+ [(if_then_else (match_operand 3 "const_int_operand" "")
+ (const_string "alu_shift_imm" )
+ (const_string "alu_shift_reg"))
+ (const_string "multiple")])]
)
(define_insn "*ifcompare_move_not"
@@ -9750,7 +9771,12 @@
[(set_attr "conds" "use")
(set_attr "shift" "2")
(set_attr "length" "4,8,8")
- (set_attr "type" "mov_shift_reg,multiple,multiple")]
+ (set_attr_alternative "type"
+ [(if_then_else (match_operand 3 "const_int_operand" "")
+ (const_string "mov_shift" )
+ (const_string "mov_shift_reg"))
+ (const_string "multiple")
+ (const_string "multiple")])]
)
(define_insn "*ifcompare_move_shift"
@@ -9788,7 +9814,12 @@
[(set_attr "conds" "use")
(set_attr "shift" "2")
(set_attr "length" "4,8,8")
- (set_attr "type" "mov_shift_reg,multiple,multiple")]
+ (set_attr_alternative "type"
+ [(if_then_else (match_operand 3 "const_int_operand" "")
+ (const_string "mov_shift" )
+ (const_string "mov_shift_reg"))
+ (const_string "multiple")
+ (const_string "multiple")])]
)
(define_insn "*ifcompare_shift_shift"
@@ -10869,7 +10900,7 @@
[(set_attr "predicable" "yes")
(set_attr "predicable_short_it" "no")
(set_attr "length" "4")
- (set_attr "type" "mov_imm")]
+ (set_attr "type" "alu_sreg")]
)
(define_insn "*arm_rev"
diff --git a/gcc/config/arm/thumb2.md b/gcc/config/arm/thumb2.md
index 1f68147..4f9faac 100644
--- a/gcc/config/arm/thumb2.md
+++ b/gcc/config/arm/thumb2.md
@@ -300,7 +300,7 @@
ldr%?\\t%0, %1
str%?\\t%1, %0
str%?\\t%1, %0"
- [(set_attr "type" "mov_reg,alu_imm,alu_imm,alu_imm,mov_imm,load1,load1,store1,store1")
+ [(set_attr "type" "mov_reg,mov_imm,mov_imm,mvn_imm,mov_imm,load1,load1,store1,store1")
(set_attr "length" "2,4,2,4,4,4,4,4,4")
(set_attr "predicable" "yes")
(set_attr "predicable_short_it" "yes,no,yes,no,no,no,no,no,no")
@@ -486,12 +486,12 @@
)
(define_insn_and_split "*thumb2_movsicc_insn"
- [(set (match_operand:SI 0 "s_register_operand" "=l,l,r,r,r,r,r,r,r,r,r")
+ [(set (match_operand:SI 0 "s_register_operand" "=l,l,r,r,r,r,r,r,r,r,r,r")
(if_then_else:SI
(match_operator 3 "arm_comparison_operator"
[(match_operand 4 "cc_register" "") (const_int 0)])
- (match_operand:SI 1 "arm_not_operand" "0 ,lPy,0 ,0,rI,K,rI,rI,K ,K,r")
- (match_operand:SI 2 "arm_not_operand" "lPy,0 ,rI,K,0 ,0,rI,K ,rI,K,r")))]
+ (match_operand:SI 1 "arm_not_operand" "0 ,lPy,0 ,0,rI,K,I ,r,rI,K ,K,r")
+ (match_operand:SI 2 "arm_not_operand" "lPy,0 ,rI,K,0 ,0,rI,I,K ,rI,K,r")))]
"TARGET_THUMB2"
"@
it\\t%D3\;mov%D3\\t%0, %2
@@ -504,12 +504,14 @@
#
#
#
+ #
#"
; alt 6: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
- ; alt 7: ite\\t%d3\;mov%d3\\t%0, %1\;mvn%D3\\t%0, #%B2
- ; alt 8: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mov%D3\\t%0, %2
- ; alt 9: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mvn%D3\\t%0, #%B2
- ; alt 10: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
+ ; alt 7: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
+ ; alt 8: ite\\t%d3\;mov%d3\\t%0, %1\;mvn%D3\\t%0, #%B2
+ ; alt 9: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mov%D3\\t%0, %2
+ ; alt 10: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mvn%D3\\t%0, #%B2
+ ; alt 11: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
"&& reload_completed"
[(const_int 0)]
{
@@ -540,10 +542,30 @@
operands[2])));
DONE;
}
- [(set_attr "length" "4,4,6,6,6,6,10,10,10,10,6")
- (set_attr "enabled_for_depr_it" "yes,yes,no,no,no,no,no,no,no,no,yes")
+ [(set_attr "length" "4,4,6,6,6,6,10,8,10,10,10,6")
+ (set_attr "enabled_for_depr_it" "yes,yes,no,no,no,no,no,no,no,no,no,yes")
(set_attr "conds" "use")
- (set_attr "type" "multiple")]
+ (set_attr_alternative "type"
+ [(if_then_else (match_operand 2 "const_int_operand" "")
+ (const_string "mov_imm")
+ (const_string "mov_reg"))
+ (if_then_else (match_operand 1 "const_int_operand" "")
+ (const_string "mov_imm")
+ (const_string "mov_reg"))
+ (if_then_else (match_operand 2 "const_int_operand" "")
+ (const_string "mov_imm")
+ (const_string "mov_reg"))
+ (const_string "mvn_imm")
+ (if_then_else (match_operand 1 "const_int_operand" "")
+ (const_string "mov_imm")
+ (const_string "mov_reg"))
+ (const_string "mvn_imm")
+ (const_string "multiple")
+ (const_string "multiple")
+ (const_string "multiple")
+ (const_string "multiple")
+ (const_string "multiple")
+ (const_string "multiple")])]
)
(define_insn "*thumb2_movsfcc_soft_insn"
@@ -1182,7 +1204,11 @@
"
[(set_attr "predicable" "yes")
(set_attr "length" "2")
- (set_attr "type" "alu_sreg")]
+ (set_attr_alternative "type"
+ [(if_then_else (match_operand 2 "const_int_operand" "")
+ (const_string "alu_imm")
+ (const_string "alu_sreg"))
+ (const_string "alu_imm")])]
)
(define_insn "*thumb2_subsi_short"
@@ -1247,14 +1273,21 @@
"
[(set_attr "conds" "set")
(set_attr "length" "2,2,4")
- (set_attr "type" "alu_sreg")]
+ (set_attr_alternative "type"
+ [(if_then_else (match_operand 2 "const_int_operand" "")
+ (const_string "alus_imm")
+ (const_string "alus_sreg"))
+ (const_string "alus_imm")
+ (if_then_else (match_operand 2 "const_int_operand" "")
+ (const_string "alus_imm")
+ (const_string "alus_sreg"))])]
)
(define_insn "*thumb2_addsi3_compare0_scratch"
[(set (reg:CC_NOOV CC_REGNUM)
(compare:CC_NOOV
- (plus:SI (match_operand:SI 0 "s_register_operand" "l,l, r,r")
- (match_operand:SI 1 "arm_add_operand" "Pv,l,IL,r"))
+ (plus:SI (match_operand:SI 0 "s_register_operand" "l, r")
+ (match_operand:SI 1 "arm_add_operand" "lPv,rIL"))
(const_int 0)))]
"TARGET_THUMB2"
"*
@@ -1271,8 +1304,10 @@
return \"cmn\\t%0, %1\";
"
[(set_attr "conds" "set")
- (set_attr "length" "2,2,4,4")
- (set_attr "type" "alus_imm,alus_sreg,alus_imm,alus_sreg")]
+ (set_attr "length" "2,4")
+ (set (attr "type") (if_then_else (match_operand 2 "const_int_operand" "")
+ (const_string "alus_imm")
+ (const_string "alus_sreg")))]
)
(define_insn "*thumb2_mulsi_short"
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH, ARM] Alternatives and type attributes fixes.
2015-04-28 8:55 ` Yvan Roux
@ 2015-04-28 14:47 ` Kyrill Tkachov
0 siblings, 0 replies; 5+ messages in thread
From: Kyrill Tkachov @ 2015-04-28 14:47 UTC (permalink / raw)
To: Yvan Roux; +Cc: gcc-patches, Ramana Radhakrishnan, Richard Earnshaw
On 28/04/15 09:32, Yvan Roux wrote:
> Hi,
>
> On 27 April 2015 at 15:58, Yvan Roux <yvan.roux@linaro.org> wrote:
>> On 27 April 2015 at 15:20, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
>>> Hi Yvan,
>>>
>>>
>>> On 27/04/15 13:25, Yvan Roux wrote:
>>>> Hi,
>>>>
>>>> This is a follow-up of PR64208 where an LRA loop was due to redundancy
>>>> in insn's alternatives. I've checked all the insns in the arm backend
>>>> to avoid latent problems and this patch fixes the issues I've spotted.
>>>>
>>>> Only thumb2_movsicc_insn contained duplicated alternatives, and the
>>>> rest of the changes are related to the type attribute, which were not
>>>> accurate or used accordingly to their definition. Notice that the
>>>> modifications have only a limited impact as in most of the pipeline
>>>> descriptions, multiple/mov_reg/alu_reg/.. types are used the same way,
>>>> only cortex a8 seems to have a real difference between alu or multiple
>>>> instruction and mov.
>>>>
>>>> Bootstrapped and regtested on arm-linux-gnueabihf. Ok for trunk ?
>>>>
>>>> Thanks,
>>>> Yvan
>>>>
>>>> 2015-04-27 Yvan Roux<yvan.roux@linaro.org>
>>>>
>>>> * config/arm/arm.mb (*arm_movt): Fix type attribute.
>>>> (*movsi_compare0): Likewise.
>>>> (*cmpsi_shiftsi): Likewise.
>>>> (*cmpsi_shiftsi_swp): Likewise.
>>>> (*movsicc_insn): Likewise.
>>>> (*cond_move): Likewise.
>>>> (*if_plus_move): Likewise.
>>>> (*if_move_plus): Likewise.
>>>> (*if_arith_move): Likewise.
>>>> (*if_move_arith): Likewise.
>>>> (*if_shift_move): Likewise.
>>>> (*if_move_shift): Likewise.
>>>> * config/arm/thumb2.md (*thumb2_movsi_insn): Fix type attribute.
>>>> (*thumb2_movsicc_insn): Fix alternative redundancy.
>>>> (*thumb2_addsi_short): Split register and immediate alternatives.
>>>> (thumb2_addsi3_compare0): Likewise.
>>>>
>>>> insn.diff
>>>>
>>>>
>>>> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
>>>> index 164ac13..ee23deb 100644
>>>> --- a/gcc/config/arm/arm.md
>>>> +++ b/gcc/config/arm/arm.md
>>>> @@ -5631,7 +5631,7 @@
>>>> [(set_attr "predicable" "yes")
>>>> (set_attr "predicable_short_it" "no")
>>>> (set_attr "length" "4")
>>>> - (set_attr "type" "mov_imm")]
>>>> + (set_attr "type" "alu_sreg")]
>>>> )
>>>
>>> For some context, this is the *arm_movt pattern that generates
>>> a movt instruction. The documentation in types.md says:
>>> "This excludes MOV and MVN but includes MOVT." So using alu_sreg
>>> is correct according to that logic.
>>> However, don't you want to also update the arm_movtas_ze pattern
>>> that generates movt but erroneously has the type 'mov_imm'?
>> Ha, yes I missed this one ! I'll will update it.
>>
>>>> (define_insn "*arm_movsi_insn"
>>>> @@ -5919,7 +5919,7 @@
>>>> cmp%?\\t%0, #0
>>>> sub%.\\t%0, %1, #0"
>>>> [(set_attr "conds" "set")
>>>> - (set_attr "type" "alus_imm,alus_imm")]
>>>> + (set_attr "type" "alus_sreg,alus_sreg")]
>>>> )
>>>
>>> Why change that? They are instructions with immediate operands
>>> so alus_imm should be gorrect?
>> Oups, I certainly misread #0 and %0 this one is correct.
>>
>>>> @@ -486,12 +486,12 @@
>>>> )
>>>> (define_insn_and_split "*thumb2_movsicc_insn"
>>>> - [(set (match_operand:SI 0 "s_register_operand"
>>>> "=l,l,r,r,r,r,r,r,r,r,r")
>>>> + [(set (match_operand:SI 0 "s_register_operand"
>>>> "=l,l,r,r,r,r,r,r,r,r,r,r")
>>>> (if_then_else:SI
>>>> (match_operator 3 "arm_comparison_operator"
>>>> [(match_operand 4 "cc_register" "") (const_int 0)])
>>>> - (match_operand:SI 1 "arm_not_operand" "0 ,lPy,0 ,0,rI,K,rI,rI,K
>>>> ,K,r")
>>>> - (match_operand:SI 2 "arm_not_operand" "lPy,0 ,rI,K,0 ,0,rI,K
>>>> ,rI,K,r")))]
>>>> + (match_operand:SI 1 "arm_not_operand" "0 ,lPy,0 ,0,rI,K,I ,r,rI,K
>>>> ,K,r")
>>>> + (match_operand:SI 2 "arm_not_operand" "lPy,0 ,rI,K,0 ,0,rI,I,K
>>>> ,rI,K,r")))]
>>>> "TARGET_THUMB2"
>>>> "@
>>>> it\\t%D3\;mov%D3\\t%0, %2
>>>> @@ -504,12 +504,14 @@
>>>> #
>>>> #
>>>> #
>>>> + #
>>>> #"
>>>> ; alt 6: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
>>>> - ; alt 7: ite\\t%d3\;mov%d3\\t%0, %1\;mvn%D3\\t%0, #%B2
>>>> - ; alt 8: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mov%D3\\t%0, %2
>>>> - ; alt 9: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mvn%D3\\t%0, #%B2
>>>> - ; alt 10: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
>>>> + ; alt 7: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
>>>> + ; alt 8: ite\\t%d3\;mov%d3\\t%0, %1\;mvn%D3\\t%0, #%B2
>>>> + ; alt 9: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mov%D3\\t%0, %2
>>>> + ; alt 10: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mvn%D3\\t%0, #%B2
>>>> + ; alt 11: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
>>>> "&& reload_completed"
>>>> [(const_int 0)]
>>>> {
>>>> @@ -540,8 +542,8 @@
>>>> operands[2])));
>>>> DONE;
>>>> }
>>>> - [(set_attr "length" "4,4,6,6,6,6,10,10,10,10,6")
>>>> - (set_attr "enabled_for_depr_it" "yes,yes,no,no,no,no,no,no,no,no,yes")
>>>> + [(set_attr "length" "4,4,6,6,6,6,10,8,10,10,10,6")
>>>> + (set_attr "enabled_for_depr_it"
>>>> "yes,yes,no,no,no,no,no,no,no,no,no,yes")
>>>> (set_attr "conds" "use")
>>>> (set_attr "type" "multiple")]
>>>> )
>>>
>>> So, I think here for the first 6 alternatives we can give it a more specific
>>> type,
>>> like mov_immm or mov_reg, since you're cleaning this stuff up. The
>>> instructions in
>>> those alternatives are just a mov instruction enclosed in an IT block, so
>>> they always
>>> have to stick together anyway.
>> OK I'll change that.
>>
>>>> @@ -1161,9 +1163,9 @@
>>>> )
>>>> (define_insn "*thumb2_addsi_short"
>>>> - [(set (match_operand:SI 0 "low_register_operand" "=l,l")
>>>> - (plus:SI (match_operand:SI 1 "low_register_operand" "l,0")
>>>> - (match_operand:SI 2 "low_reg_or_int_operand" "lPt,Ps")))
>>>> + [(set (match_operand:SI 0 "low_register_operand" "=l,l,l")
>>>> + (plus:SI (match_operand:SI 1 "low_register_operand" "l,l,0")
>>>> + (match_operand:SI 2 "low_reg_or_int_operand" "l,Pt,Ps")))
>>>> (clobber (reg:CC CC_REGNUM))]
>>>> "TARGET_THUMB2 && reload_completed"
>>>> "*
>>>> @@ -1182,7 +1184,7 @@
>>>> "
>>>> [(set_attr "predicable" "yes")
>>>> (set_attr "length" "2")
>>>> - (set_attr "type" "alu_sreg")]
>>>> + (set_attr "type" "alu_sreg,alu_imm,alu_imm")]
>>>> )
>>>> (define_insn "*thumb2_subsi_short"
>>>> @@ -1226,10 +1228,10 @@
>>>> (define_insn "thumb2_addsi3_compare0"
>>>> [(set (reg:CC_NOOV CC_REGNUM)
>>>> (compare:CC_NOOV
>>>> - (plus:SI (match_operand:SI 1 "s_register_operand" "l, 0, r")
>>>> - (match_operand:SI 2 "arm_add_operand" "lPt,Ps,rIL"))
>>>> + (plus:SI (match_operand:SI 1 "s_register_operand" "l,l, 0,r,r")
>>>> + (match_operand:SI 2 "arm_add_operand"
>>>> "l,Pt,Ps,r,IL"))
>>>> (const_int 0)))
>>>> - (set (match_operand:SI 0 "s_register_operand" "=l,l,r")
>>>> + (set (match_operand:SI 0 "s_register_operand" "=l,l,l,r,r")
>>>> (plus:SI (match_dup 1) (match_dup 2)))]
>>>> "TARGET_THUMB2"
>>>> "*
>>>> @@ -1246,8 +1248,8 @@
>>>> return \"adds\\t%0, %1, %2\";
>>>> "
>>>> [(set_attr "conds" "set")
>>>> - (set_attr "length" "2,2,4")
>>>> - (set_attr "type" "alu_sreg")]
>>>> + (set_attr "length" "2,2,2,4,4")
>>>> + (set_attr "type" "alus_sreg,alus_imm,alus_imm,alus_sreg,alus_imm")]
>>>> )
>>>>
>>>
>>> In the other patterns in arm.md you didn't split up the alternatives
>>> but instead used an if_then_else in the set_attr_alternative to
>>> differentiate
>>> the case where the operand is constant.
>>> Any particular reason why you split up the alternatives here?
>> I think that I tried to be consistent with the implementation of
>> *thumb2_addsi3_compare0_scratch insn, it is also due to this work was
>> interrupted several time and I wasn't really consistent with myself ;)
>>
>>> In my opinion, having fewer alternatives at the expense of a more complex
>>> definition
>>> of 'type' is preferable, but someone might have a stronger opinion in the
>>> other
>>> direction.
>> I also prefer fewer alternatives, I'll rework the patch that way and
>> refactor *thumb2_addsi3_compare0_scratch too (or will do the opposite
>> if a good argument comes in that thread).
>>
>>> The patch looks ok to me otherwise, but please respin with the comments
>>> above addressed.
> Here is the new patch with the needed modifications. Rebuilt and
> regtested on arm-linux-gnueabihf.
This is ok.
Thanks,
Kyrill
>
> Cheers,
> Yvan
>
> 2015-04-28 Yvan Roux <yvan.roux@linaro.org>
>
> * config/arm/arm.mb (*arm_movt): Fix type attribute.
> (*cmpsi_shiftsi): Likewise.
> (*cmpsi_shiftsi_swp): Likewise.
> (*movsicc_insn): Likewise.
> (*cond_move): Likewise.
> (*if_plus_move): Likewise.
> (*if_move_plus): Likewise.
> (*if_arith_move): Likewise.
> (*if_move_arith): Likewise.
> (*if_shift_move): Likewise.
> (*if_move_shift): Likewise.
> (*arm_movtas_ze): Likewise.
> * config/arm/thumb2.md (*thumb2_movsicc_insn): Fix alternative
> redundancy and
> type attributes.
> (*thumb2_movsi_insn): Fix type attribute.
> (*thumb2_addsi_short): Likewise.
> (thumb2_addsi3_compare0): Likewise.
> (*thumb2_addsi3_compare0_scratch): Merge alternatives and fix attributes
> accordingly.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-04-28 14:31 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-27 12:25 [PATCH, ARM] Alternatives and type attributes fixes Yvan Roux
2015-04-27 13:20 ` Kyrill Tkachov
2015-04-27 13:58 ` Yvan Roux
2015-04-28 8:55 ` Yvan Roux
2015-04-28 14:47 ` Kyrill Tkachov
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).