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

[Ramana Radhakrishnan <ramana.radhakrishnan@linaro.org>]

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
>