From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32716 invoked by alias); 14 Dec 2011 17:32:35 -0000 Received: (qmail 32707 invoked by uid 22791); 14 Dec 2011 17:32:33 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,TW_IW,TW_MX,TW_VF,TW_VG,TW_XF X-Spam-Check-By: sourceware.org Received: from service87.mimecast.com (HELO service87.mimecast.com) (91.220.42.44) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 14 Dec 2011 17:32:19 +0000 Received: from cam-owa2.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.21]) by service87.mimecast.com; Wed, 14 Dec 2011 17:32:16 +0000 Received: from [10.1.69.67] ([10.1.255.212]) by cam-owa2.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.3959); Wed, 14 Dec 2011 17:32:14 +0000 Message-ID: <4EE8DD9D.3060508@arm.com> Date: Wed, 14 Dec 2011 17:48:00 -0000 From: Richard Earnshaw User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:6.0.2) Gecko/20110902 Thunderbird/6.0.2 MIME-Version: 1.0 To: Xinyu Qi CC: Ramana Radhakrishnan , "gcc-patches@gcc.gnu.org" Subject: Re: PING: [PATCH, ARM, iWMMXt][4/5]: WMMX machine description References: <4737A960563B524DA805CA602BE04B306010E1F4E9@SC-VEXCH2.marvell.com> <4737A960563B524DA805CA602BE04B30602611FB90@SC-VEXCH2.marvell.com> <4737A960563B524DA805CA602BE04B30602925062B@SC-VEXCH2.marvell.com> In-Reply-To: <4737A960563B524DA805CA602BE04B30602925062B@SC-VEXCH2.marvell.com> X-MC-Unique: 111121417321600901 Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes 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 X-SW-Source: 2011-12/txt/msg01112.txt.bz2 On 24/11/11 01:33, Xinyu Qi wrote: > Hi Ramana, >=20 > I solve the conflict, please try again. The new diff is attached. >=20 > Thanks, > Xinyu >=20 > At 2011-11-19 07:36:15,"Ramana Radhakrishnan" wrote: >> >> Hi Xinyu, >> >> This doesn't apply cleanly currently on trunk and the reject appears >> to come from iwmmxt.md and I've not yet investigated why. >> >> Can you have a look ? >> This patch is NOT ok. You're adding features that were new in iWMMXt2 (ie not in the original implementation) but you've provided no means by which the compiler can detect which operations are only available on the new cores. Further, I don't like the way you have separate patterns for the rotates with immediate. Please investigate using the alternative enable feature to reduce these down to single patterns. Once you've done all this, some of your tricky builtin expand code in patch 3/5 should then simplify significantly as you won't need separate expand codes for those alternatives. R. >> cheers >> Ramana >> >> On 26 September 2011 04:22, Xinyu Qi wrote: >>> Ping. >>> >>> http://gcc.gnu.org/ml/gcc-patches/2011-09/msg00279.html >>> >>> * config/arm/arm.c (arm_output_iwmmxt_shift_immediate): New >> function. >>> (arm_output_iwmmxt_tinsr): Likewise. >>> * config/arm/arm-protos.h >> (arm_output_iwmmxt_shift_immediate): Declare. >>> (arm_output_iwmmxt_tinsr): Likewise. >>> * config/arm/iwmmxt.md (WCGR0, WCGR1, WCGR2, WCGR3): >> New constant. >>> (iwmmxt_psadbw, iwmmxt_walign, iwmmxt_tmrc, iwmmxt_tmcr): >> Delete. >>> (iwmmxt_tbcstqi, iwmmxt_tbcsthi, iwmmxt_tbcstsi): Likewise >>> (*iwmmxt_clrv8qi, *iwmmxt_clrv4hi, *iwmmxt_clrv2si): Likewise. >>> (tbcstv8qi, tbcstv4hi, tbsctv2si): New pattern. >>> (iwmmxt_clrv8qi, iwmmxt_clrv4hi, iwmmxt_clrv2si): Likewise. >>> (*and3_iwmmxt, *ior3_iwmmxt, >> *xor3_iwmmxt): Likewise. >>> (rori3, ashri3_iwmmxt, lshri3_iwmmxt): >> Likewise. >>> (ashli3_iwmmxt, iwmmxt_waligni, iwmmxt_walignr): >> Likewise. >>> (iwmmxt_walignr0, iwmmxt_walignr1): Likewise. >>> (iwmmxt_walignr2, iwmmxt_walignr3): Likewise. >>> (iwmmxt_setwcgr0, iwmmxt_setwcgr1): Likewise. >>> (iwmmxt_setwcgr2, iwmmxt_setwcgr3): Likewise. >>> (iwmmxt_getwcgr0, iwmmxt_getwcgr1): Likewise. >>> (iwmmxt_getwcgr2, iwmmxt_getwcgr3): Likewise. >>> (All instruction patterns): Add wtype attribute. >>> (*iwmmxt_arm_movdi, *iwmmxt_movsi_insn): iWMMXt coexist >> with vfp. >>> (iwmmxt_uavgrndv8qi3, iwmmxt_uavgrndv4hi3): Revise the >> pattern. >>> (iwmmxt_uavgv8qi3, iwmmxt_uavgv4hi3): Likewise. >>> (iwmmxt_tinsrb, iwmmxt_tinsrh, iwmmxt_tinsrw):Likewise. >>> (eqv8qi3, eqv4hi3, eqv2si3, gtuv8qi3): Likewise. >>> (gtuv4hi3, gtuv2si3, gtv8qi3, gtv4hi3, gtv2si3): Likewise. >>> (iwmmxt_wunpckihh, iwmmxt_wunpckihw, iwmmxt_wunpckilh): >> Likewise. >>> (iwmmxt_wunpckilw, iwmmxt_wunpckehub, iwmmxt_wunpckehuh): >> Likewise. >>> (iwmmxt_wunpckehuw, iwmmxt_wunpckehsb, >> iwmmxt_wunpckehsh): Likewise. >>> (iwmmxt_wunpckehsw, iwmmxt_wunpckelub, >> iwmmxt_wunpckeluh): Likewise. >>> (iwmmxt_wunpckeluw, iwmmxt_wunpckelsb, iwmmxt_wunpckelsh): >> Likewise. >>> (iwmmxt_wunpckelsw, iwmmxt_wmadds, iwmmxt_wmaddu): >> Likewise. >>> (iwmmxt_wsadb, iwmmxt_wsadh, iwmmxt_wsadbz, >> iwmmxt_wsadhz): Likewise. >>> (iwmmxt2.md): Include. >>> * config/arm/iwmmxt2.md: New file. >>> * config/arm/iterators.md (VMMX2): New mode_iterator. >>> * config/arm/arm.md (wtype): New attribute. >>> (UNSPEC_WMADDS, UNSPEC_WMADDU): Delete. >>> (UNSPEC_WALIGNI): New unspec. >>> * config/arm/t-arm (MD_INCLUDES): Add iwmmxt2.md. >>> >>> At 2011-09-05 17:55:34,"Xinyu Qi" wrote: >>>> At 2011-08-18 10:21:01,"Ramana Radhakrishnan" >>>> wrote: >>>>> On 14 July 2011 08:45, Xinyu Qi wrote: >>>>>>> Hi, >>>>>>> >>>>>>> It is the fourth part of iWMMXt maintenance. >>>>>>> >>>>> >>>>> Can this be broken down further. ? I'll have to do this again but >>>>> there are some initial comments below for some discussion. >>>> >>>>> >>>>>> (*iwmmxt_arm_movdi, *iwmmxt_movsi_insn, iwmmxt_uavgrndv8qi3, >>>>> iwmmxt_uavgrndv4hi3, iwmmxt_uavgv8qi3, iwmmxt_uavgv4hi3, >> iwmmxt_tinsrb, >>>>> iwmmxt_tinsrh, iwmmxt_tinsrw, eqv8qi3, eqv4hi3, eqv2si3, gtuv8qi3, >>>> gtuv4hi3, >>>>> gtuv2si3, gtv8qi3, gtv4hi3, gtv2si3, iwmmxt_wunpckihb, >> iwmmxt_wunpckihh, >>>>> iwmmxt_wunpckihw, iwmmxt_wunpckilb, iwmmxt_wunpckilh, >> iwmmxt_wunpckilw, >>>>> iwmmxt_wunpckehub, iwmmxt_wunpckehuh, iwmmxt_wunpckehuw, >>>> iwmmxt_wunpckehsb, >>>>> iwmmxt_wunpckehsh, iwmmxt_wunpckehsw, iwmmxt_wunpckelub, >>>> iwmmxt_wunpckeluh, >>>>> iwmmxt_wunpckeluw, iwmmxt_wunpckelsb, iwmmxt_wunpckelsh, >>>> iwmmxt_wunpckelsw, >>>>> iwmmxt_wmadds, iwmmxt_wmaddu, iwmmxt_wsadb, iwmmxt_wsadh, >> iwmmxt_wsadbz, >>>>> iwmmxt_wsadhz): Revise. >>>>> >>>>> Revise to do what ? >>>> >>>> Sorry for late response. >>>> >>>> Some of them have incorrect RTL templates. For example, see >> iwmmxt_uavgv8qi3 >>>> Its old RTL template is: >>>> [(set (match_operand:V8QI 0 "register_operand" >> "=3Dy") >>>> (ashiftrt:V8QI (plus:V8QI >>>> (match_operand:V8QI 1 >> "register_operand" "y") >>>> (match_operand:V8QI 2 >> "register_operand" "y")) >>>> (const_int 1)))] >>>> >>>> According to the assembly behavior of wavg2b, the correct one should b= e: >>>> [(set (match_operand:V8QI 0 "register_operand" "=3Dy") >>>> (truncate:V8QI >>>> (lshiftrt:V8HI >>>> (plus:V8HI (zero_extend:V8HI (match_operand:V8QI 1 >>>> "register_operand" "y")) >>>> (zero_extend:V8HI (match_operand:V8QI 2 >>>> "register_operand" "y"))) >>>> (const_int 1))))] >>>> >>>> Consider the case: >>>> The Operation on element 0x01 and 0xff: gcc with old RTL template would >> optimize >>>> to the result 0x00.That is: >>>> 0x01 + 0xff =3D> 0x00. 0x00 > 1 =3D> 0x00 >>>> While the correct result should be 0x80. >>>> 0x01 =3D> 0x0001, 0xff =3D> 0x00ff. 0x0001 + 0x00ff =3D> 0x0100. 0x010= 0 > 1 =3D> >> 0x0080, >>>> 0x0080 =3D> 0x80 >>>> >>>> iwmmxt_wmadds and iwmmxt_wmaddu are modified to use detailed RTL >> template >>>> instead of unspec. >>>> >>>> For some of the wunpck patterns, change the order of zero_extend and >> vec_select >>>> in order to avoid a vec_select optimization internal error in old vers= ion gcc. >>>> Maybe this internal bug has been fixed, but such modification is harml= ess. >>>> >>>> Rests of them are only revised for their format. >>>> >>>>> >>>>>> (define_insn "*iwmmxt_movsi_insn" >>>>>> - [(set (match_operand:SI 0 "nonimmediate_operand" "=3Drk,r,r,rk, >>>>> m,z,r,?z,Uy,z") >>>>>> - (match_operand:SI 1 "general_operand" "rk, >> I,K,mi,rk,r,z,Uy,z, >>>>> z"))] >>>>>> + [(set (match_operand:SI 0 "nonimmediate_operand" "=3Drk,r,r,r,rk, >>>>> m,z,r,?z,?Uy,?z,t,r,?t,?z,t") >>>>>> + (match_operand:SI 1 "general_operand" " >> rk,I,K,N,mi,rk,r,z,Uy, >>>> z, >>>>> z,r,t, z, t,t"))] >>>>>> "TARGET_REALLY_IWMMXT >>>>>> - && ( register_operand (operands[0], SImode) >>>>>> - || register_operand (operands[1], SImode))" >>>>>> - "* >>>>>> - switch (which_alternative) >>>>>> + && ((register_operand (operands[0], SImode) >>>>>> + && (!reload_completed >>>>>> + || REGNO_REG_CLASS (REGNO (operands[0])) =3D=3D >> IWMMXT_GR_REGS)) >>>>>> + || (register_operand (operands[1], SImode) >>>>>> + && (!reload_completed >>>>> >>>>> >>>>> >>>>>> + || REGNO_REG_CLASS (REGNO (operands[1])) =3D=3D >> IWMMXT_GR_REGS)))" >>>>> >>>>> I don't like this at all - what you are doing is assuming that after >>>>> reg-alloc you are going to be able to rely on whether something has a >>>>> particular register class and then turn on and off it's matching. So >>>>> this matches before reload and doesn't do so after reload for the >>>>> cases where *iwmmxt_movsi_insn is really in a core register. I don't >>>>> think you can do it this way. If you really want to do this properly - >>>>> have an arch field for iwmmxt as well in the arch attribute and then >>>>> add these alternatives to existing patterns. >>>>> >>>>> If I understand what you are trying to do here - you are trying to use >>>>> *arm_movsi_insn and other patterns in the rest of the backend and let >>>>> things like "predicable" kick in right after reload for all cases >>>>> other than the ones you enumerate. In which case get rid of all the >>>>> other constaints in this pattern other than the constraints that are >>>>> valid for registers of class IWMMXT_REGS >>>> >>>> This piece of code is added to make iwmmxt coexist with vfp when iwmmxt >> and >>>> vfp are enabled together. Agree, I don't think it is a good fix. >>>> Add adequate constrains to *iwmmxt_movsi_insn and >> *iwmmxt_arm_movdi so that >>>> don't need to change their conditions. >>>> >>>>> >>>>> Also the definition of output_move_double has changed now and hence >>>>> this needs some rework. >>>> >>>> Done. >>>> >>>>> Should there be a distinction between iwmmxt and iwmmxt2 ? Is it a >>>>> user visible option ? >>>> >>>> I don't think users need to know the distinction between iwmmxt and >> iwmmxt2 >>>> though there are two options "-mcpu=3Diwmmxt" and "-mcpu=3Diwmmxt2". It >> seems if >>>> "-mcpu=3Diwmmxt" is specified in gcc, the assembler cannot recognize t= he >> wmmx2 >>>> insns. >>>> >>>> >>>>> >>>>> Just in case it wasn't clear please don't commit any patch in this >>>>> series until all the patches have been completely reviewed. >>>>> >>>>> cheers >>>>> Ramana >>>> >>>> >>>> The new diff attached. New ChangLog: >>>> >>>> * config/arm/arm.c (arm_output_iwmmxt_shift_immediate): New function. >>>> (arm_output_iwmmxt_tinsr): Likewise. >>>> * config/arm/arm-protos.h (arm_output_iwmmxt_shift_immediate): >> Declare. >>>> (arm_output_iwmmxt_tinsr): Likewise. >>>> * config/arm/iwmmxt.md (WCGR0, WCGR1, WCGR2, WCGR3): New >> constant. >>>> (iwmmxt_psadbw, iwmmxt_walign, iwmmxt_tmrc, iwmmxt_tmcr): >> Delete. >>>> (iwmmxt_tbcstqi, iwmmxt_tbcsthi, iwmmxt_tbcstsi): Likewise >>>> (*iwmmxt_clrv8qi, *iwmmxt_clrv4hi, *iwmmxt_clrv2si): Likewise. >>>> (tbcstv8qi, tbcstv4hi, tbsctv2si): New pattern. >>>> (iwmmxt_clrv8qi, iwmmxt_clrv4hi, iwmmxt_clrv2si): Likewise. >>>> (*and3_iwmmxt, *ior3_iwmmxt, >> *xor3_iwmmxt): Likewise. >>>> (rori3, ashri3_iwmmxt, lshri3_iwmmxt): >> Likewise. >>>> (ashli3_iwmmxt, iwmmxt_waligni, iwmmxt_walignr): Likewise. >>>> (iwmmxt_walignr0, iwmmxt_walignr1, iwmmxt_walignr2, >> iwmmxt_walignr3): >>>> Likewise. >>>> (iwmmxt_setwcgr0, iwmmxt_setwcgr1, iwmmxt_setwcgr2, >> iwmmxt_setwcgr3): >>>> Likewise. >>>> (iwmmxt_getwcgr0, iwmmxt_getwcgr1, iwmmxt_getwcgr2, >> iwmmxt_getwcgr3): >>>> Likewise. >>>> (All instruction patterns): Add wtype attribute. >>>> (*iwmmxt_arm_movdi, *iwmmxt_movsi_insn): iWMMXt coexist with >> vfp. >>>> (iwmmxt_uavgrndv8qi3, iwmmxt_uavgrndv4hi3): Revise the pattern. >>>> (iwmmxt_uavgv8qi3, iwmmxt_uavgv4hi3): Likewise. >>>> (iwmmxt_tinsrb, iwmmxt_tinsrh, iwmmxt_tinsrw):Likewise. >>>> (eqv8qi3, eqv4hi3, eqv2si3, gtuv8qi3): Likewise. >>>> (gtuv4hi3, gtuv2si3, gtv8qi3, gtv4hi3, gtv2si3): Likewise. >>>> (iwmmxt_wunpckihh, iwmmxt_wunpckihw, iwmmxt_wunpckilh): >> Likewise. >>>> (iwmmxt_wunpckilw, iwmmxt_wunpckehub, iwmmxt_wunpckehuh): >> Likewise. >>>> (iwmmxt_wunpckehuw, iwmmxt_wunpckehsb, iwmmxt_wunpckehsh): >> Likewise. >>>> (iwmmxt_wunpckehsw, iwmmxt_wunpckelub, iwmmxt_wunpckeluh): >> Likewise. >>>> (iwmmxt_wunpckeluw, iwmmxt_wunpckelsb, iwmmxt_wunpckelsh): >> Likewise. >>>> (iwmmxt_wunpckelsw, iwmmxt_wmadds, iwmmxt_wmaddu): Likewise. >>>> (iwmmxt_wsadb, iwmmxt_wsadh, iwmmxt_wsadbz, iwmmxt_wsadhz): >> Likewise. >>>> (iwmmxt2.md): Include. >>>> * config/arm/iwmmxt2.md: New file. >>>> * config/arm/iterators.md (VMMX2): New mode_iterator. >>>> * config/arm/arm.md (wtype): New attribute. >>>> (UNSPEC_WMADDS, UNSPEC_WMADDU): Delete. >>>> (UNSPEC_WALIGNI): New unspec. >>>> * config/arm/t-arm (MD_INCLUDES): Add iwmmxt2.md. >>>> >>>> Thanks, >>>> Xinyu >> >