Re: [Patch]: Splitter for Altivec moves

[Aldy Hernandez <aldyh at redhat dot com>]

[rth: questions below]

> I reworked patch and tested it with a cross powerpc-eabialtivec
> configuration.

Did you test with the little endian multilibs?  Just curious, because
I think there are endianness issues with your patch.

Also, could you please come up with a testcase that triggers at least
one of the conditions, ideally all of them?

>       * config/rs6000/rs6000.c (rs6000_split_altivec_in_gprs): New function.
>       (altivec_in_gprs_p): New function.
>       * config/rs6000/rs6000-protos (rs6000_split_altivec_in_gprs): New
>       prototype.
>       (altivec_in_gprs_p): New prototype.
>       * config/rs6000/altivec.md (*movv4si_internal): Change
>       multi-assembler alternative to '#'. Add postreload splitter to
>       handle this cases.
>       (*movv4hi_internal): Likewise.
>       (*movv4qi_internal): Likewise.
>       (*movv4sf_internal): Likewise.

You need to add a space after each file ChangeLog entry.  Ala:

    * some_file: blah

    * some_other_file: blah
    
> + extern void rs6000_split_altivec_in_gprs (rtx*);

Space before '*'.

> + /* Return 1 for all valid move insn operand combination involving altivec
> +    vectors in gprs.  */
> +
> + int
> + altivec_in_gprs_p (rtx op0, rtx op1)
> + {
> +   if (REG_P (op0) && !ALTIVEC_REGNO_P (REGNO (op0)))
> +     return 1;
> +
> +   if (REG_P (op1) && !ALTIVEC_REGNO_P (REGNO (op1)))
> +     return 1;
> +   return 0;

How about an FPR?  An FPR is a register, but it's not a AltiVec
register.  The constraint in the original insn should make guarantee
we get either a GPR or an AltiVec register by the time we get here,
but it's better to do things right.  We may want to use
altivec_in_gprs_p() for something else later. You want something like:

  if (REG_P (op0) && REGNO_REG_CLASS (REGNO (op0)) == GENERAL_REGS)

> +    This is done, if a gprs is at least one of the operands. In this case

"This is done if GPRs are one of the operands..."

> +    a sequence of simple move insns has to be issued. The sequence of this

A couple syntax problems, plus you need a 2 spaces after a period:

"a sequence of simple move insns have to be issued.  The sequence of
these...".

> +   nregs = HARD_REGNO_NREGS(reg, mode);

Need a space before "(".

> +   else
> +     {
> +       j = -1;
> +
> +       if (GET_CODE (operands[1]) == MEM)
> +         {
> +           rtx breg;
> +           /* We have offsettable addresses only. If we use one of the
> +              registers to address memory, we have change that register last.  */
> +           breg = GET_CODE (XEXP (operands[1], 0)) == PLUS ?
> +               XEXP (XEXP (operands[1], 0), 0) :
> +               XEXP (operands[1], 0);
> +
> +           if (REGNO (breg) >= REGNO (operands[0])
> +               && REGNO (breg) < REGNO (operands[0]) + nregs)
> +               j = REGNO (breg) - REGNO (operands[0]);
> +         }
> +
> +       for (i = 0; i < nregs; i++)
> +         {
> +           /* Calculate index to next subword.  */
> +           j++;
> +           if (j == nregs)
> +             j = 0;
> +
> +           operands[i + 2] = operand_subword (operands[0], j, 0, mode);
> +           operands[i + 2 + nregs] =
> +             operand_subword (operands[1], j, 0, mode);
> +
> +         }
> +     }
> + }

I'm almost sure your operand_subword() call will have endianness
issues when we do MEM->GPRs or MEM<-GPRs.

Rth, can you comment on this?

Aldy