Message ID | CAD57uCdxCK-D8eHgEQ4KLbipC1h0Rhb==yFd__C4MANa5X8n5A@mail.gmail.com |
---|---|
State | New |
Headers | show |
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.
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"