Re: altivec patches

[Aldy Hernandez <aldyh@redhat.com>]

>>>>> "Geoff" == Geoff Keating <geoffk@geoffk.org> writes:

 >> ! #define BIGGEST_ALIGNMENT (TARGET_ALTIVEC_ABI ? 128 : 64)

 > I think you _don't_ want this to be dependent on the ABI.  Changing
 > this isn't an ABI change (I hope!), which is good because...

Fixed.  Set to 128 unconditionally.

 >> ! 
 >> ! /* A C expression to compute the alignment for a variables in the
 >> !    local store.  TYPE is the data type, and ALIGN is the alignment
 >> !    that the object would ordinarily have.  */
 >> ! #define LOCAL_ALIGNMENT(TYPE, ALIGN)				\
 >> ! 	((TARGET_ALTIVEC_ABI		    			\
 >> ! 	  && TREE_CODE (TYPE)) == VECTOR_TYPE ? 128 : ALIGN)

 > ..as altivec values have to be 128-bit aligned because of the
 > hardware, you don't want this ABI-dependent either.

Fixed.  Set to TARGET_ALTIVEC.  Actually, that's ambiguous because you
shouldn't have vector types unless you are in altivec mode, but it
won't hurt.

 > The DWARF_FRAME_REGISTERS change seems to have gone away, which is not
 > quite right:  it has to happen _only when the ABI is 'altivec'_.

I took it out because it needs rethinking.  DWARF_FRAME_REGISTERS is
used as an array size in a few places, particulary unwind-dw2.c.  So I
need to change all these places to dynamically allocate their space.

 > This will be a bit tricky because DWARF_FRAME_REGISTERS is also used
 > in libgcc, so you have to #define something when the Altivec ABI is
 > chosen, and then key off that if IN_LIBGCC2.

 > I wonder if you could assume that the integrated cpplib will always be
 > used when compiling libgcc?  That would make things easier, because
 > then you wouldn't have muck with specs so much.

Please explain.  I only see DWARF_FRAME_REGISTERS being used in
dwarf2out.c and unwind-dw2.c.  There is nothing in libgcc*.

 >> !    /* AltiVec registers.  */			   \
 >> !    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, \
 >> !    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0  \
 >> ! }

 > This is correct, but doesn't there need to be a change to them in
 > CONDITIONAL_REGISTER_USAGE when the altivec ABI is chosen (in which
 > some altivec registers are call-saved)?

And there already is: :)

  if (TARGET_ALTIVEC_ABI)
    for (i = FIRST_ALTIVEC_REGNO; i < FIRST_ALTIVEC_REGNO + 20; ++i)
      call_used_regs[i] = 1;

 > These should be based off FIRST_ALTIVEC_REGISTER rather than being
 > magic numbers...

Fixed.

 >> /* Return registers */
 >> #define GP_ARG_RETURN GP_ARG_MIN_REG
 >> #define FP_ARG_RETURN FP_ARG_MIN_REG
 >> + #define ALTIVEC_ARG_RETURN 79

 > .. this too.

Fixed.

 >> #undef	STACK_BOUNDARY
 >> ! #define	STACK_BOUNDARY	64
 >> 
 >> /* Real stack boundary as mandated by the appropriate ABI.  */
 >> ! #define ABI_STACK_BOUNDARY ((TARGET_EABI) ? 64 : 128)
 >> 
 >> /* No data type wants to be aligned rounder than this.  */
 >> #undef	BIGGEST_ALIGNMENT
 >> ! #define BIGGEST_ALIGNMENT ((TARGET_EABI) ? 64 : 128)
 >> 
 >> #undef  BIGGEST_FIELD_ALIGNMENT
 >> #undef  ADJUST_FIELD_ALIGN
 >> - #undef  ROUND_TYPE_ALIGN
 >> 
 >> /* Use ELF style section commands.  */
 >> 
 >> --- 400,430 ----
 >> one set of libraries with -mno-eabi instead of eabi libraries and non-eabi
 >> versions, just use 64 as the stack boundary.  */
 >> #undef	STACK_BOUNDARY
 >> ! #define	STACK_BOUNDARY	(TARGET_ALTIVEC_ABI ? 128 : 64)
 >> 
 >> /* Real stack boundary as mandated by the appropriate ABI.  */
 >> ! #define ABI_STACK_BOUNDARY ((TARGET_EABI && !TARGET_ALTIVEC_ABI) ? 64 : 128)
 >> 
 >> /* No data type wants to be aligned rounder than this.  */
 >> #undef	BIGGEST_ALIGNMENT
 >> ! #define BIGGEST_ALIGNMENT ((TARGET_EABI && !TARGET_ALTIVEC_ABI) ? 64 : 128)
 >> 
 >> + /* An expression for the alignment of a structure field FIELD if the
 >> +    alignment computed in the usual way is COMPUTED.  */
 >> + #define ADJUST_FIELD_ALIGN(FIELD, COMPUTED)				      \
 >> + 	((TARGET_ALTIVEC_ABI && TREE_CODE (TREE_TYPE (FIELD)) == VECTOR_TYPE) \
 >> + 	 ? 128 : COMPUTED)
 >> + 
 >> + /* Define this macro as an expression for the alignment of a type
 >> +    (given by TYPE as a tree node) if the alignment computed in the
 >> +    usual way is COMPUTED and the alignment explicitly specified was
 >> +    SPECIFIED.  */
 >> + #define ROUND_TYPE_ALIGN(TYPE, COMPUTED, SPECIFIED)			\
 >> + 	((TARGET_ALTIVEC_ABI && TREE_CODE (TYPE) == VECTOR_TYPE)	\
 >> + 	 ? 128 : MAX (COMPUTED, SPECIFIED))
 >> + 
 >> #undef  BIGGEST_FIELD_ALIGNMENT
 >> #undef  ADJUST_FIELD_ALIGN
 >> 
 >> /* Use ELF style section commands.  */

 > This has the same comment as the equivalent stuff in rs6000.h.

Huh?  What do you mean?

 >> *************** function_arg_advance (cum, mode, type, n
 >> *** 2159,2165 ****
 >> {
 cum-> nargs_prototype--;
 >> 
 >> !   if (DEFAULT_ABI == ABI_V4 || DEFAULT_ABI == ABI_SOLARIS)
 >> {
 >> if (TARGET_HARD_FLOAT
 >> && (mode == SFmode || mode == DFmode))
 >> --- 2242,2255 ----
 >> {
 cum-> nargs_prototype--;
 >> 
 >> !   if (TARGET_ALTIVEC_ABI && ALTIVEC_VECTOR_MODE (mode))
 >> !     {
 >> !       if (cum->vregno <= ALTIVEC_ARG_MAX_REG)
 >> ! 	cum->vregno++;
 >> !       else
 >> ! 	cum->words += RS6000_ARG_SIZE (mode, type);
 >> !     }

 > Something you might not have seen in the ABI documentation: unnamed
 > vector arguments (passed to varargs functions after the '...') get
 > passed in memory, not in registers.  (I don't remember if this is just
 > the unnamed arguments or all the arguments to varargs functions, you
 > should check.)

This is just for unnamed arguments in varargs.  So I'll have to add:

        if (cum->vregno <= ALTIVEC_ARG_MAX_REG && cum->nargs_prototype >= 0)
                                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
done. (?)

 >> +       /* Print AltiVec memory operand.  */
 >> +     case 'y':
 >> +       {
 >> + 	rtx tmp;
 >> + 
 >> + 	if (GET_CODE (x) != MEM)
 >> + 	  abort ();
 >> + 
 >> + 	tmp = XEXP (x, 0);
 >> + 
 >> + 	if (GET_CODE (tmp) == REG)
 >> + 	  fprintf (file, "0, %s", reg_names[REGNO (tmp)]);
 >> + 	else if (GET_CODE (tmp) == PLUS && GET_CODE (XEXP (tmp, 1)) == REG)
 >> + 	  {
 >> + 	    if (REGNO (XEXP (tmp, 0)) == 0)
 >> + 	      fprintf (file, "%s,%s", reg_names[ REGNO (XEXP (tmp, 1)) ],
 >> + 		       reg_names[ REGNO (XEXP (tmp, 0)) ]);
 >> + 	    else
 >> + 	      fprintf (file, "%s,%s", reg_names[ REGNO (XEXP (tmp, 0)) ],
 >> + 		       reg_names[ REGNO (XEXP (tmp, 1)) ]);
 >> + 	  }
 >> + 	else
 >> + 	  abort ();
 >> + 	break;
 >> +       }

 > Do you really need this?  It looks just like the way a normal memory
 > operand is printed (for the cases that are allowed for altivec).

Yes I do.  I want "[reg+reg]" to be outputed as "reg,reg" and [reg] to
be outputed as "reg,0".

Consider "lvx d,a,b".  I want to match:

        (set (reg) (mem))

and have (mem) be dumped as "a,b" (the two registers in the memory
address: (plus (reg) (reg))).  And I make sure that addresses get
canonicalized into (mem (reg)) and (mem (plus (reg) (reg))) with
LEGITIM* code.

 > Can you put the altivec insns in their own .md file and use the include
 > mechanism?  I believe this is what it was intended for.

As soon as you approve Alan's patch, yes :).

 >> + ;; No code is needed to copy between vector registers.
 >> + (define_insn "*altivec_vec_move"
 >> +   [(set (match_operand 0 "altivec_register" "=v")
 >> + 	(match_operand 1 "altivec_register" "v"))]
 >> +   "TARGET_ALTIVEC"
 >> +   ""
 >> +   [(set_attr "type" "altivec")])

 > Um, that can't possibly be right.  Also, it should be part of
 > movv4si_internal, because reload needs all the move insns to be the
 > same pattern.

What the hell was that all about?  I should get my cvs access revoked
for that one.  I dunno how that make it there.  M-x doctor

 >> + ;; Vector move instructions.
 >> + (define_expand "movv4si"
 >> +   [(set (match_operand:V4SI 0 "nonimmediate_operand" "")
 >> + 	(match_operand:V4SI 1 "any_operand" ""))]
 >> +   "TARGET_ALTIVEC"
 >> +   "{ rs6000_emit_move (operands[0], operands[1], V4SImode); DONE; }")
 >> + 
 >> + (define_insn "*movv4si_internal"
 >> +   [(set (match_operand:V4SI 0 "nonimmediate_operand" "=m,v")
 >> + 	(match_operand:V4SI 1 "input_operand" "v,m"))]
 >> +   "TARGET_ALTIVEC"
 >> +   "@
 >> +    stvx\t%1,%y0
 >> +    ldvx\t%0,%y1"
 >> +   [(set_attr "type" "altivec")])

 > Yes---this needs an alternative to copy a 'v' into a 'v'.  All the
 > ones below probably need it too.

Will do.

Aldy