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.r...@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
