Re: [IRIX 6/Ultrix V4] Non-MIPS-ABI conformant passing of small structures

[Rainer Orth <ro at TechFak dot Uni-Bielefeld dot DE>]

Jim,

thanks alot for the thorough explanation of the issues with my patch.

> >  I simply
> >  removed the test for !TARGET_64BIT since this convention is valid both
> >  for the o32 and n32/n64 ABIs, as described in 
> 
> This changes the o64 ABI which is unnecessary and undesirable.  I'd suggest
> changing the ! TARGET_64BIT to mips_abi != ABI_O64 which matches the original
> intent.  This will likely require an additional va-mips.h change, but we can
> worry about that later.

The new patch below should take care of this via FUNCTION_ARG_PADDING.
I've left va-mips.h as before, since I've no way to test a fix.

> I'd suggest forgetting about the archaic and ugly left-shift structure code
> in the mips function_arg function, and instead fix the problem by making
> function_arg return a PARALLEL for the n32/n64 ABIs when the argument is
> a structure which is not a multiple of 8 bytes, and will be passed entirely
> in registers.  I think this is a more elegant solution, and it does a much
> better job of fixing the general problem.  We probably need a similar change
> for mips_function_value to handle structure return values.  We still have
> an efficiency problem, but at least it is now in the emit_group_load/store
> functions, which are easier to change than the general argument handling
> code in calls.c/function.c.

The patch below does this.  I've used the sparc port as a guide, which
pointed me to the FUNCTION_ARG_PADDING change.  The PARALLEL support was
mostly there in mips.c (function_arg), I had just to fix two bugs: the test
for a struct consisting only of a double field was backwards, so the
PARALLEL code was never executed.  The calculation of the number of chunks
didn't account for structs not a multiple of the word size.  This could
lead to 0 chunks and a PARALLEL without the EXPR_LISTS, crashing the
compiler.

I've removed the old left-shifting code and could remove num_adjusts and
adjusts in CUMULATIVE_ARGS as they are now unused.

va-mips.h is unchanged from the previous version.

I noticed a typo in the FUNCTION_ARG description in tm.texi, which is fixed
as well.

Btw., I noticed that the mips port still uses gen_rtx (FOO, ... instead of
gen_rtx_FOO.  I suppose this could be mechanically replaced?

With this change, the compiler bootstraps correctly on IRIX 6.2 (both ABIs)
and passes the updated version of my tests, included below, with two
exceptions:

* in the sa-gcc-{cc, gcc} cases (caller passing struct to variadic function
  compiled with gcc, callee compiled with either cc or gcc), an 8-bit
  struct consisting of a single char isn't passed correctly, while the
  similar case of passing the same struct to a `regular' function is ok.

  I noticed that this struct is passed as QImode to function_arg, unlike
  all other structures which are BLKmode.  May this be related to the
  problem?

  At the moment, I have no idea how to resolve this problem, and this bug
  must certainly be fixed since gcc is even inconsistent with itself.

* in the st-cc-gcc case (caller passing structs compiled with cc,
  callee with gcc), several structs aren't fetched correctly.  gcc is
  consistent with itself, though.  Since a similar problem in this
  combination of caller and callee occurs on a bi-arch 32/64-bit gcc on
  Solaris 7 in sparcv9 mode, I suppose it's a generic problem (in
  emit_group_load?).

  correct:

  s16 = 8|16
  s32 = 8|16|24|32
  s48 = 8|16|24|32|40|48
  s72 = 8|16|24|32|40|48|56|64|72
  s80 = 8|16|24|32|40|48|56|64|72|80
  s96i = 32|64|96

  mips-sgi-irix6.2, n32:

  s16 = 1|0
  s32 = 1|0|0|2
  s48 = 1|0|0|2|0|0
  s72 = 1|0|0|2|0|0|0|4|0
  s80 = 1|0|0|2|0|0|0|4|0|0
  s96i = 16777218|4|4

  sparc-sun-solaris2.7, sparcv9:

  s16 = 8|3
  s32 = 8|3|0|0
  s48 = 8|3|0|0|0|0
  s72 = 8|16|24|32|40|48|56|64|0
  s80 = 8|16|24|32|40|48|56|64|0|0
  s96i = 32|674248768|0

	Rainer


Fri Jun 25 01:24:46 1999  Rainer Orth  <ro@TechFak.Uni-Bielefeld.DE>

	* tm.texi (FUNCTION_ARG): Fixed typo.

	* mips/mips.c (function_arg): Fixed test for struct consisting
	of only a double.
	Correctly compute chunks even for structs not a multiple of the
	word size.
	Removed obsolete code to left-align small structures and return
	adjustment rtx.
	* mips/mips.h (CUMULATIVE_ARGS): Removed num_adjusts, adjusts
	fields, unnecessary. 
	* mips/abi64.h (FUNCTION_ARG_PADDING): Left-align structures on
	big-endian targets.

Tue Jun 22 19:42:10 1999  Rainer Orth  <ro@TechFak.Uni-Bielefeld.DE>

	* ginclude/va-mips.h (va-arg): Removed wrong special treatment of
	__mips64.  Left-align structures on big-endian targets only.

Index: config/mips/abi64.h
===================================================================
RCS file: /egcs/carton/cvsfiles/egcs/gcc/config/mips/abi64.h,v
retrieving revision 1.11
diff -u -p -r1.11 abi64.h
--- abi64.h	1999/03/22 18:51:14	1.11
+++ abi64.h	1999/06/25 19:34:54
@@ -73,8 +73,11 @@ Boston, MA 02111-1307, USA.  */
    ? (MAX_ARGS_IN_REGISTERS*UNITS_PER_WORD) - FIRST_PARM_OFFSET (FNDECL) \
    : 0)
 
+/* Structures are left-aligned on big-endian targets, except in the o64 ABI,
+   which keeps right-alignment for efficiency.  */
 #define FUNCTION_ARG_PADDING(MODE, TYPE)				\
   (! BYTES_BIG_ENDIAN							\
+   || (mips_abi != ABI_O64 && (TYPE) && AGGREGATE_TYPE_P (TYPE))	\
    ? upward								\
    : (((MODE) == BLKmode						\
        ? ((TYPE) && TREE_CODE (TYPE_SIZE (TYPE)) == INTEGER_CST		\
Index: config/mips/mips.c
===================================================================
RCS file: /egcs/carton/cvsfiles/egcs/gcc/config/mips/mips.c,v
retrieving revision 1.56
diff -u -p -r1.56 mips.c
--- mips.c	1999/05/07 12:42:03	1.56
+++ mips.c	1999/06/25 19:34:57
@@ -3836,7 +3836,7 @@ function_arg (cum, mode, type, named)
 
 	  /* If the whole struct fits a DFmode register,
 	     we don't need the PARALLEL.  */
-	  if (! field || mode == DFmode)
+	  if (field && mode == DFmode)
 	    ret = gen_rtx (REG, mode, regbase + *arg_words + bias);
 	  else
 	    {
@@ -3847,10 +3847,8 @@ function_arg (cum, mode, type, named)
 	      int regno;
 	      int i;
 
-	      /* ??? If this is a packed structure, then the last hunk won't
-		 be 64 bits.  */
-
-	      chunks = TREE_INT_CST_LOW (TYPE_SIZE (type)) / BITS_PER_WORD;
+	      chunks = (TREE_INT_CST_LOW (TYPE_SIZE (type))
+			+ BITS_PER_WORD - 1) / BITS_PER_WORD;
 	      if (chunks + *arg_words + bias > MAX_ARGS_IN_REGISTERS)
 		chunks = MAX_ARGS_IN_REGISTERS - *arg_words - bias;
 
@@ -3893,54 +3891,14 @@ function_arg (cum, mode, type, named)
       if (TARGET_DEBUG_E_MODE)
 	fprintf (stderr, "%s%s\n", reg_names[regbase + *arg_words + bias],
 		 struct_p ? ", [struct]" : "");
-
-      /* The following is a hack in order to pass 1 byte structures
-	 the same way that the MIPS compiler does (namely by passing
-	 the structure in the high byte or half word of the register).
-	 This also makes varargs work.  If we have such a structure,
-	 we save the adjustment RTL, and the call define expands will
-	 emit them.  For the VOIDmode argument (argument after the
-	 last real argument), pass back a parallel vector holding each
-	 of the adjustments.  */
-
-      /* ??? function_arg can be called more than once for each argument.
-	 As a result, we compute more adjustments than we need here.
-	 See the CUMULATIVE_ARGS definition in mips.h.  */
-
-      /* ??? This scheme requires everything smaller than the word size to
-	 shifted to the left, but when TARGET_64BIT and ! TARGET_INT64,
-	 that would mean every int needs to be shifted left, which is very
-	 inefficient.  Let's not carry this compatibility to the 64 bit
-	 calling convention for now.  */
-
-      if (struct_p && int_size_in_bytes (type) < UNITS_PER_WORD
-	  && ! TARGET_64BIT && mips_abi != ABI_EABI)
-	{
-	  rtx amount = GEN_INT (BITS_PER_WORD
-				- int_size_in_bytes (type) * BITS_PER_UNIT);
-	  rtx reg = gen_rtx (REG, word_mode, regbase + *arg_words + bias);
-
-	  if (TARGET_64BIT)
-	    cum->adjust[cum->num_adjusts++] = gen_ashldi3 (reg, reg, amount);
-	  else
-	    cum->adjust[cum->num_adjusts++] = gen_ashlsi3 (reg, reg, amount);
-	}
     }
 
   /* We will be called with a mode of VOIDmode after the last argument
      has been seen.  Whatever we return will be passed to the call
-     insn.  If we need any shifts for small structures, return them in
-     a PARALLEL; in that case, stuff the mips16 fp_code in as the
-     mode.  Otherwise, if we have need a mips16 fp_code, return a REG
-     with the code stored as the mode.  */
-  if (mode == VOIDmode)
-    {
-      if (cum->num_adjusts > 0)
-	ret = gen_rtx (PARALLEL, (enum machine_mode) cum->fp_code,
-		       gen_rtvec_v (cum->num_adjusts, cum->adjust));
-      else if (TARGET_MIPS16 && cum->fp_code != 0)
-	ret = gen_rtx (REG, (enum machine_mode) cum->fp_code, 0);
-    }
+     insn.  If we need a mips16 fp_code, return a REG with the code stored
+     as the mode.  */
+  if (mode == VOIDmode && TARGET_MIPS16 && cum->fp_code != 0)
+    ret = gen_rtx (REG, (enum machine_mode) cum->fp_code, 0);
 
   return ret;
 }
Index: config/mips/mips.h
===================================================================
RCS file: /egcs/carton/cvsfiles/egcs/gcc/config/mips/mips.h,v
retrieving revision 1.54.4.1
diff -u -p -r1.54.4.1 mips.h
--- mips.h	1999/05/19 23:05:45	1.54.4.1
+++ mips.h	1999/06/25 19:34:59
@@ -2508,12 +2508,6 @@ typedef struct mips_args {
   int fp_arg_words;		/* # words for FP args (MIPS_EABI only) */
   int last_arg_fp;		/* nonzero if last arg was FP (EABI only) */
   int fp_code;			/* Mode of FP arguments (mips16) */
-  int num_adjusts;		/* number of adjustments made */
-				/* Adjustments made to args pass in regs.  */
-				/* ??? The size is doubled to work around a 
-				   bug in the code that sets the adjustments
-				   in function_arg.  */
-  struct rtx_def *adjust[MAX_ARGS_IN_REGISTERS*2];
 } CUMULATIVE_ARGS;
 
 /* Initialize a variable CUM of type CUMULATIVE_ARGS
Index: config/ginclude/va-mips.h
===================================================================
RCS file: /egcs/carton/cvsfiles/egcs/gcc/ginclude/va-mips.h,v
retrieving revision 1.5
diff -u -p -r1.5 va-mips.h
--- va-mips.h	1998/12/16 21:19:36	1.5
+++ va-mips.h	1999/06/25 19:35:00
@@ -233,41 +233,23 @@ void va_end (__gnuc_va_list);		/* Define
 
 /* We cast to void * and then to TYPE * because this avoids
    a warning about increasing the alignment requirement.  */
-/* The __mips64 cases are reversed from the 32 bit cases, because the standard
-   32 bit calling convention left-aligns all parameters smaller than a word,
-   whereas the __mips64 calling convention does not (and hence they are
-   right aligned).  */
-#ifdef __mips64
 #ifdef __MIPSEB__
-#define va_arg(__AP, __type)                                    \
-  ((__type *) (void *) (__AP = (char *)                         \
-                       ((((__PTRDIFF_TYPE__)__AP + 8 - 1) & -8) \
-			   + __va_rounded_size (__type))))[-1]
-#else
-#define va_arg(__AP, __type)                                    \
-  ((__AP = (char *) ((((__PTRDIFF_TYPE__)__AP + 8 - 1) & -8)	\
-		     + __va_rounded_size (__type))),		\
-   *(__type *) (void *) (__AP - __va_rounded_size (__type)))
-#endif
-
-#else /* not __mips64 */
-
-#ifdef __MIPSEB__
 /* For big-endian machines.  */
 #define va_arg(__AP, __type)					\
   ((__AP = (char *) ((__alignof__ (__type) > 4			\
 		      ? ((__PTRDIFF_TYPE__)__AP + 8 - 1) & -8	\
 		      : ((__PTRDIFF_TYPE__)__AP + 4 - 1) & -4)	\
 		     + __va_rounded_size (__type))),		\
-   *(__type *) (void *) (__AP - __va_rounded_size (__type)))
+   (__builtin_classify_type (*(__type *) 0) < __record_type_class	\
+   ? ((__type *) (void *) __AP)[-1]					\
+   : *(__type *) (void *) (__AP - __va_rounded_size (__type))))
 #else
 /* For little-endian machines.  */
 #define va_arg(__AP, __type)						    \
   ((__type *) (void *) (__AP = (char *) ((__alignof__(__type) > 4	    \
 				? ((__PTRDIFF_TYPE__)__AP + 8 - 1) & -8	    \
 				: ((__PTRDIFF_TYPE__)__AP + 4 - 1) & -4)    \
-					 + __va_rounded_size(__type))))[-1]
-#endif
+					 + sizeof(__type))))[-1]
 #endif
 #endif /* ! defined (__mips_eabi)  */
 
Index: tm.texi
===================================================================
RCS file: /egcs/carton/cvsfiles/egcs/gcc/tm.texi,v
retrieving revision 1.75.4.1
diff -u -p -r1.75.4.1 tm.texi
--- tm.texi	1999/05/31 02:56:15	1.75.4.1
+++ tm.texi	1999/06/25 19:44:16
@@ -2786,7 +2786,7 @@ pushed, zero suffices as a definition.
 
 The value of the expression can also be a @code{parallel} RTX.  This is
 used when an argument is passed in multiple locations.  The mode of the
-of the @code{parallel} should be the mode of the entire argument.  The
+@code{parallel} should be the mode of the entire argument.  The
 @code{parallel} holds any number of @code{expr_list} pairs; each one
 describes where part of the argument is passed.  In each
 @code{expr_list} the first operand must be a @code{reg} RTX for the hard

mips-struct.tar.gz