From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 122017 invoked by alias); 27 Apr 2015 13:20:28 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 122001 invoked by uid 89); 27 Apr 2015 13:20:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.2 X-HELO: eu-smtp-delivery-143.mimecast.com Received: from eu-smtp-delivery-143.mimecast.com (HELO eu-smtp-delivery-143.mimecast.com) (146.101.78.143) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 27 Apr 2015 13:20:25 +0000 Received: from cam-owa2.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.140]) by uk-mta-19.uk.mimecast.lan; Mon, 27 Apr 2015 14:20:23 +0100 Received: from [10.2.207.50] ([10.1.2.79]) by cam-owa2.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.3959); Mon, 27 Apr 2015 14:20:22 +0100 Message-ID: <553E3796.7060204@arm.com> Date: Mon, 27 Apr 2015 13:20:00 -0000 From: Kyrill Tkachov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Yvan Roux , "gcc-patches@gcc.gnu.org" , Ramana Radhakrishnan , Richard Earnshaw Subject: Re: [PATCH, ARM] Alternatives and type attributes fixes. References: In-Reply-To: X-MC-Unique: Y0StYaigSGuS28IQBNmnAQ-1 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2015-04/txt/msg01616.txt.bz2 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 > > * 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'? >=20=20=20 > (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 @@ > ) >=20=20=20 > (define_insn_and_split "*thumb2_movsicc_insn" > - [(set (match_operand:SI 0 "s_register_operand" "=3Dl,l,r,r,r,r,r,r,r,r= ,r") > + [(set (match_operand:SI 0 "s_register_operand" "=3Dl,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,y= es") > (set_attr "conds" "use") > (set_attr "type" "multiple")] > ) So, I think here for the first 6 alternatives we can give it a more specifi= c type, like mov_immm or mov_reg, since you're cleaning this stuff up. The instruct= ions in those alternatives are just a mov instruction enclosed in an IT block, so t= hey always have to stick together anyway. > @@ -1161,9 +1163,9 @@ > ) >=20=20=20 > (define_insn "*thumb2_addsi_short" > - [(set (match_operand:SI 0 "low_register_operand" "=3Dl,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" "=3Dl,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")] > ) >=20=20=20 > (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" "=3Dl,l,r") > + (set (match_operand:SI 0 "s_register_operand" "=3Dl,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")] > ) >=20=20=20 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 differentia= te 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 d= efinition of 'type' is preferable, but someone might have a stronger opinion in the o= ther direction. The patch looks ok to me otherwise, but please respin with the comments abo= ve addressed. Thanks for cleaning this up! Kyrill