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

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

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.

>
> Since "*cond_iwmmxt_movsi_insn" would be got rid of soon, I keep it unchanged.
>
> *config/arm/arm.c (arm_output_iwmmxt_shift_immediate): New function.
>  (arm_output_iwmmxt_tinsr): Ditto.

s/Ditto/Likewise

> *config/arm/arm-protos.h (arm_output_iwmmxt_shift_immediate): New prototype.
s/New prototype/Declare.

>  (arm_output_iwmmxt_tinsr): Ditto.
> *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, *iwmmxt_clrv8qi, *iwmmxt_clrv4hi, *iwmmxt_clrv2si): Rename...



>  (tbcstv8qi, tbcstv4hi, tbsctv2si, iwmmxt_clrv8qi, iwmmxt_clrv4hi, iwmmxt_clrv2si): ...New pattern.
s/...//

>  (*and<mode>3_iwmmxt, *ior<mode>3_iwmmxt, *xor<mode>3_iwmmxt, rori<mode>3, ashri<mode>3_iwmmxt, lshri<mode>3_iwmmxt, ashli<mode>3_iwmmxt, iwmmxt_waligni, iwmmxt_walignr, iwmmxt_walignr0, iwmmxt_walignr1, iwmmxt_walignr2, iwmmxt_walignr3, iwmmxt_setwcgr0, iwmmxt_setwcgr1, iwmmxt_setwcgr2, iwmmxt_setwcgr3, iwmmxt_getwcgr0, iwmmxt_getwcgr1, iwmmxt_getwcgr2, iwmmxt_getwcgr3): New pattern.

Break this up into multiple lines. Each line should only have upto 80
characters. Put this on one line and say New and add the others in a
row afterwards and use Likewise.

Look at other patches in the near past which set up Changelogs.


>  (*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 ?

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



>  (iwmmxt2.md): Include.
> *config/arm/iwmmxt2.md: New file.
> *config/arm/iterators.md (VMMX2): New mode_iterator.
> *config/arm/arm.md (wtype): New attribute.
> *config/arm/t-arm (MD_INCLUDES): Add iwmmxt2.md.
>
> Thanks,
> Xinyu
>


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


The documentation states with respect to conditions for a define_insn :

`For nameless patterns, the condition is applied only when matching an
individual insn, and only after the insn has matched the pattern's
recognition template.  The insn's operands may be found in the vector
@code{operands}.  For an insn where the condition has once matched, it
can't be used to control register allocation, for example by excluding
certain hard registers or hard register combinations.'

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

Also the definition of output_move_double has changed now and hence
this needs some rework.

Should there be a distinction between iwmmxt and iwmmxt2 ? Is it a
user visible option ?

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