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

[Richard Earnshaw <rearnsha@arm.com>]

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.

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