RE: PING: [PATCH, ARM, iWMMXt][4/5]: WMMX machine description

[Xinyu Qi <xyqi@marvell.com>]

[-- Attachment #1: Type: text/plain, Size: 16950 bytes --]

At 2011-12-15 01:32:13,"Richard Earnshaw" <rearnsha@arm.com> wrote:
> On 24/11/11 01:33, Xinyu Qi wrote:
> > Hi Ramana,
> >
> > I solve the conflict, please try again. The new diff is attached.
> >
> > Thanks,
> > Xinyu
> >
> > At 2011-11-19 07:36:15,"Ramana Radhakrishnan"
> <ramana.radhakrishnan@linaro.org> 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.

Hi Richard,

All of the WMMX chips support WMMX2 instructions.
What I do is to complement the WMMX2 intrinsic support in GCC.
I don't think it is necessary for users to consider whether one WMMX insn is a WMMX2 insn or not.

> 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.

Done. Combine immediate and register alternative into one pattern for all shift patterns.
Also update the part three. Send it in another mail.
The new diff is attached. The 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.
        (rorv4hi3, rorv2si3, rordi3): Likewise.
        (rorv4hi3_di, rorv2si3_di, rordi3_di): Likewise.
        (ashrv4hi3_di, ashrv2si3_di, ashrdi3_di): Likewise.
        (lshrv4hi3_di, lshrv2si3_di, lshrdi3_di): Likewise.
        (ashlv4hi3_di, ashlv2si3_di, ashldi3_di): Likewise.
        (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.
        (*and<mode>3_iwmmxt, *ior<mode>3_iwmmxt, *xor<mode>3_iwmmxt): Likewise.
        (ror<mode>3, ror<mode>3_di): Likewise.
        (ashr<mode>3_di, lshr<mode>3_di, ashl<mode>3_di): Likewise.
        (ashli<mode>3_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.
        (ashr<mode>3_iwmmxt, ashl<mode>3_iwmmxt, lshr<mode>3_iwmmxt): 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.
        * config/arm/predicates.md (imm_or_reg_operand): New predicate.

Thanks,
Xinyu

> R.
>
> >> cheers
> >> Ramana
> >>
> >> On 26 September 2011 04:22, Xinyu Qi <xyqi@marvell.com> 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.
> >>>        (*and<mode>3_iwmmxt, *ior<mode>3_iwmmxt,
> >> *xor<mode>3_iwmmxt): Likewise.
> >>>        (rori<mode>3, ashri<mode>3_iwmmxt, lshri<mode>3_iwmmxt):
> >> Likewise.
> >>>        (ashli<mode>3_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" <xyqi@marvell.com> wrote:
> >>>> At 2011-08-18 10:21:01,"Ramana Radhakrishnan"
> >>>> <ramana.radhakrishnan@linaro.org> wrote:
> >>>>> On 14 July 2011 08:45, Xinyu Qi <xyqi@marvell.com> 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"
> >> "=y")
> >>>>         (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 be:
> >>>>   [(set (match_operand:V8QI  0 "register_operand" "=y")
> >>>>         (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 => 0x00. 0x00 > 1 => 0x00
> >>>> While the correct result should be 0x80.
> >>>> 0x01 => 0x0001, 0xff => 0x00ff. 0x0001 + 0x00ff => 0x0100. 0x0100 > 1 =>
> >> 0x0080,
> >>>> 0x0080 => 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 version
> gcc.
> >>>> Maybe this internal bug has been fixed, but such modification is harmless.
> >>>>
> >>>> Rests of them are only revised for their format.
> >>>>
> >>>>>
> >>>>>> (define_insn "*iwmmxt_movsi_insn"
> >>>>>> -  [(set (match_operand:SI 0 "nonimmediate_operand" "=rk,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" "=rk,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])) ==
> >> IWMMXT_GR_REGS))
> >>>>>> +       || (register_operand (operands[1], SImode)
> >>>>>> +     && (!reload_completed
> >>>>>
> >>>>>
> >>>>>
> >>>>>> +         || REGNO_REG_CLASS (REGNO (operands[1])) ==
> >> 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=iwmmxt" and "-mcpu=iwmmxt2".
> It
> >> seems if
> >>>> "-mcpu=iwmmxt" is specified in gcc, the assembler cannot recognize the
> >> 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.
> >>>>   (*and<mode>3_iwmmxt, *ior<mode>3_iwmmxt,
> >> *xor<mode>3_iwmmxt): Likewise.
> >>>>   (rori<mode>3, ashri<mode>3_iwmmxt, lshri<mode>3_iwmmxt):
> >> Likewise.
> >>>>   (ashli<mode>3_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
> >> >
>


[-- Attachment #2: 4_wmmx_md.diff.gz --]
[-- Type: application/x-gzip, Size: 13077 bytes --]