| Message ID | CAD57uCfboBEqPmP-kamsDoymis=C26DO7+Q1LFGLDSTN5=Ea2w@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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 >
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"
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.