This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
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:
insn2.diff
Description: Text document
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |