On 28/04/15 09:32, Yvan Roux wrote:
Hi,On 27 April 2015 at 15:58, Yvan Roux <yvan.r...@linaro.org> wrote:On 27 April 2015 at 15:20, Kyrill Tkachov <kyrylo.tkac...@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.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'?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.r...@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.
