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

[Xinyu Qi <xyqi at marvell dot com>]

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 ?
> 
> 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: 4_wmmx_md.diff.gz
Description: 4_wmmx_md.diff.gz