RFC: MIPS o32 ABI fix and o64 ABI change

[Richard Sandiford <rsandifo at redhat dot com>]

Sigh.  Time to confess.  In the 3.2->3.3 cycle, I reworked the MIPS
argument-passing code in order to fix several (M)EABI problems:

   http://gcc.gnu.org/ml/gcc-patches/2002-03/msg01340.html

Unfortunately, the patch also introduced a bug in the way o32 is handled.

The bug in question affects functions that take two "float" arguments,
followed by at least one other argument of any type.  For example:

    void foo (float a, float b, int c, int d)

The register dispositions are supposed to be:

        a: $f12
        b: $f14
        c: $6
        d: $7

but 3.3.x and 3.4.x use:

        a: $f12
        b: $f14
        c: $7
        d: stack

The way the argument code is written, the natural place for "b" would be
$f13, because "b" is the second word of the conceptual argument structure
and $f13 is the second floating-point argument register.  Special code is
needed to select $f14 instead.

The problem was that the code to select $f14 was in the wrong place.
It would effectively treat $f13 as an extra word of the argument
structure, thus affecting the position of "c" and "d" above.
The simple patch below fixes this.

First of all, do you (meaning Eric specifically, but any interested
party in general) agree that this bug should be fixed?  Some reasons
for doing so are:

   - it's needed for IRIX compatiblity
   - certain ffi-style code knows what the correct ABI is
   - gdb still implements the correct ABI

but it will of course mean more ABI churn.

If the o32 bug is fixed, the follow-on question is: what should we do
about o64?  Before the patch linked above, the o64 code would pass a
second _float_ argument in $f14 but a second _double_ in $f13.  I'm
pretty sure this was unintentional, but I tried to keep compatiblity
with it when writing the patch above.

Unfortunately, o64 is affected by the same third-argument bug as o32.
Both before and after my patch, the o64 register dispositions for
foo() were/are the same as those for o32, so we get "c" and "d"
wrong from a compatibility point of view.

If we're fixing the o32 case, I'd like to get rid of the o64
wart once and for all, and pass both floats and doubles in $f13.
This is also done by the patch below.  Do you agree that that's
a good idea?  Again, gdb seems to implement the expected ABI,
with $f13 holding both float and double arguments.

The patch also fixes a problem with the way va_start() tracked the stack
location of floats for EABI32.  At the moment, it incorrectly rounds
the address of floating-point stack arguments to FP_INC * UNITS_PER_FPREG,
something that should only happen for register arguments.  (Note that when
it comes to EABI, such rounding is unnecessary for register arguments too,
since fp_offset is always a multiple of FP_INC.)  Testcase attached.

BTW, the EABI part isn't really an ABI change, because the caller and
callee already agree on where the named arguments are.  The bug was
entirely local to va_start().

The patch was bootstrapped & regression tested on mips-sgi-irix6.5.
I added a SKIP_COMPLEX define to gcc.dg/compat (because I only have
access to a version of IRIX without c99 complex support) and then
tested compatiblity with MIPSpro using:

    -DSKIP_COMPLEX
    -DSKIP_ATTRIBUTE
    -DSKIP_ZERO_ARRAY

for all three ABIs.  After this patch, and the one I'm about to post,
the results were:

n32:
    FAIL: gcc.dg/compat/struct-by-value-10 c_compat_x_tst.o-c_compat_y_alt.o execute

    FAIL: gcc.dg/compat/struct-by-value-10 c_compat_x_alt.o-c_compat_y_tst.o execute

    FAIL: gcc.dg/compat/struct-by-value-10 c_compat_x_alt.o-c_compat_y_alt.o execute

    FAIL: gcc.dg/compat/struct-by-value-22 c_compat_x_alt.o compile
    FAIL: gcc.dg/compat/struct-by-value-22 c_compat_y_alt.o compile
    FAIL: gcc.dg/compat/struct-return-10 c_compat_x_tst.o-c_compat_y_alt.o execute 
    FAIL: gcc.dg/compat/struct-return-10 c_compat_x_alt.o-c_compat_y_tst.o execute 
    FAIL: gcc.dg/compat/struct-return-10 c_compat_x_alt.o-c_compat_y_alt.o execute 

n64:

    FAIL: gcc.dg/compat/struct-by-value-10 c_compat_x_tst.o-c_compat_y_alt.o execute

    FAIL: gcc.dg/compat/struct-by-value-10 c_compat_x_alt.o-c_compat_y_tst.o execute

    FAIL: gcc.dg/compat/struct-by-value-10 c_compat_x_alt.o-c_compat_y_alt.o execute

    FAIL: gcc.dg/compat/struct-by-value-22 c_compat_x_alt.o compile
    FAIL: gcc.dg/compat/struct-by-value-22 c_compat_y_alt.o compile
    FAIL: gcc.dg/compat/struct-return-10 c_compat_x_tst.o-c_compat_y_alt.o execute 
    FAIL: gcc.dg/compat/struct-return-10 c_compat_x_alt.o-c_compat_y_tst.o execute 
    FAIL: gcc.dg/compat/struct-return-10 c_compat_x_alt.o-c_compat_y_alt.o execute 

o32:

    FAIL: gcc.dg/compat/struct-by-value-22 c_compat_x_alt.o compile
    FAIL: gcc.dg/compat/struct-by-value-22 c_compat_y_alt.o compile

The struct-by-value-10 and struct-return-10 failures are due to a bug
in the MIPSpro handling of long doubles, see:

    http://gcc.gnu.org/ml/gcc-patches/2003-08/msg01483.html

for details.  Notice that the (alt,alt) combination, which uses just
MIPSpro, also fails.  struct-by-value-22 fails to compile with MIPSpro
because it uses VLAs.

I also tested the patch on mipsisa64-elf ({-mips32,-mip64}{-EL,-EB})
to get EABI coverage.  The patch fixes the attached testcase and
introduces no regressions.

Eric, is this OK with you?

Richard


	* config/mips/mips.h (struct mips_args): Clarify comments.
	* config/mips/mips.c (struct mips_arg_info): Likewise.
	(mips_arg_info): Don't allow fpr_p to affect the register or
	stack alignment.  Remove o64 silliness.
	(function_arg): Deal with the o32 float,float case specially.

testsuite/
	* gcc.c-torture/execute/va-arg-26.c: New test.

Index: config/mips/mips.h
===================================================================
RCS file: /cvs/gcc/gcc/gcc/config/mips/mips.h,v
retrieving revision 1.370
diff -c -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.370 mips.h
*** config/mips/mips.h	18 Sep 2004 19:19:37 -0000	1.370
--- config/mips/mips.h	22 Sep 2004 06:21:57 -0000
*************** #define FUNCTION_ARG_REGNO_P(N)					\
*** 2180,2190 ****
     && !fixed_regs[N])
  
  /* This structure has to cope with two different argument allocation
!    schemes.  Most MIPS ABIs view the arguments as a struct, of which the
!    first N words go in registers and the rest go on the stack.  If I < N,
!    the Ith word might go in Ith integer argument register or the
!    Ith floating-point one.  For these ABIs, we only need to remember
!    the number of words passed so far.
  
     The EABI instead allocates the integer and floating-point arguments
     separately.  The first N words of FP arguments go in FP registers,
--- 2180,2190 ----
     && !fixed_regs[N])
  
  /* This structure has to cope with two different argument allocation
!    schemes.  Most MIPS ABIs view the arguments as a structure, of which
!    the first N words go in registers and the rest go on the stack.  If I
!    < N, the Ith word might go in Ith integer argument register or in a
!    floating-point register.  For these ABIs, we only need to remember
!    the offset of the current argument into the structure.
  
     The EABI instead allocates the integer and floating-point arguments
     separately.  The first N words of FP arguments go in FP registers,
*************** typedef struct mips_args {
*** 2212,2220 ****
    /* The number of arguments seen so far.  */
    unsigned int arg_number;
  
!   /* For EABI, the number of integer registers used so far.  For other
!      ABIs, the number of words passed in registers (whether integer
!      or floating-point).  */
    unsigned int num_gprs;
  
    /* For EABI, the number of floating-point registers used so far.  */
--- 2212,2220 ----
    /* The number of arguments seen so far.  */
    unsigned int arg_number;
  
!   /* The number of integer registers used so far.  For all ABIs except
!      EABI, this is the number of words that have been added to the
!      argument structure, limited to MAX_ARGS_IN_REGISTERS.  */
    unsigned int num_gprs;
  
    /* For EABI, the number of floating-point registers used so far.  */
Index: config/mips/mips.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/config/mips/mips.c,v
retrieving revision 1.469
diff -c -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.469 mips.c
*** config/mips/mips.c	13 Sep 2004 19:32:04 -0000	1.469
--- config/mips/mips.c	22 Sep 2004 06:22:07 -0000
*************** struct mips_arg_info
*** 419,426 ****
    /* The number of words passed in registers, rounded up.  */
    unsigned int reg_words;
  
!   /* The offset of the first register from GP_ARG_FIRST or FP_ARG_FIRST,
!      or MAX_ARGS_IN_REGISTERS if the argument is passed entirely
       on the stack.  */
    unsigned int reg_offset;
  
--- 419,430 ----
    /* The number of words passed in registers, rounded up.  */
    unsigned int reg_words;
  
!   /* For EABI, the offset of the first register from GP_ARG_FIRST or
!      FP_ARG_FIRST.  For other ABIs, the offset of the first register from
!      the start of the ABI's argument structure (see the CUMULATIVE_ARGS
!      comment for details).
! 
!      The value is MAX_ARGS_IN_REGISTERS if the argument is passed entirely
       on the stack.  */
    unsigned int reg_offset;
  
*************** static void
*** 3046,3052 ****
  mips_arg_info (const CUMULATIVE_ARGS *cum, enum machine_mode mode,
  	       tree type, int named, struct mips_arg_info *info)
  {
!   bool even_reg_p;
    unsigned int num_bytes, num_words, max_regs;
  
    /* Work out the size of the argument.  */
--- 3050,3056 ----
  mips_arg_info (const CUMULATIVE_ARGS *cum, enum machine_mode mode,
  	       tree type, int named, struct mips_arg_info *info)
  {
!   bool doubleword_aligned_p;
    unsigned int num_bytes, num_words, max_regs;
  
    /* Work out the size of the argument.  */
*************** mips_arg_info (const CUMULATIVE_ARGS *cu
*** 3123,3149 ****
        gcc_unreachable ();
      }
  
!   /* Now decide whether the argument must go in an even-numbered register.
!      Usually this is determined by type alignment, but there are two
!      exceptions:
! 
!      - Under the O64 ABI, the second float argument goes in $f14 if it
!        is single precision (doubles go in $f13 as expected).
! 
!      - Floats passed in FPRs must be in an even-numbered register if
!        we're using paired FPRs.  */
!   if (type)
!     even_reg_p = TYPE_ALIGN (type) > BITS_PER_WORD;
!   else
!     even_reg_p = GET_MODE_UNIT_SIZE (mode) > UNITS_PER_WORD;
! 
!   if (info->fpr_p)
!     {
!       if (mips_abi == ABI_O64 && mode == SFmode)
! 	even_reg_p = true;
!       if (FP_INC > 1)
! 	even_reg_p = true;
!     }
  
    /* Set REG_OFFSET to the register count we're interested in.
       The EABI allocates the floating-point registers separately,
--- 3127,3136 ----
        gcc_unreachable ();
      }
  
!   /* See whether the argument has doubleword alignment.  */
!   doubleword_aligned_p = (type
! 			  ? TYPE_ALIGN (type) > BITS_PER_WORD
! 			  : GET_MODE_UNIT_SIZE (mode) > UNITS_PER_WORD);
  
    /* Set REG_OFFSET to the register count we're interested in.
       The EABI allocates the floating-point registers separately,
*************** mips_arg_info (const CUMULATIVE_ARGS *cu
*** 3152,3163 ****
  		      ? cum->num_fprs
  		      : cum->num_gprs);
  
!   if (even_reg_p)
      info->reg_offset += info->reg_offset & 1;
  
!   /* The alignment applied to registers is also applied to stack arguments.  */
    info->stack_offset = cum->stack_words;
!   if (even_reg_p)
      info->stack_offset += info->stack_offset & 1;
  
    max_regs = MAX_ARGS_IN_REGISTERS - info->reg_offset;
--- 3139,3151 ----
  		      ? cum->num_fprs
  		      : cum->num_gprs);
  
!   /* Advance to an even register if the argument is doubleword-aligned.  */
!   if (doubleword_aligned_p)
      info->reg_offset += info->reg_offset & 1;
  
!   /* Work out the offset of a stack argument.  */
    info->stack_offset = cum->stack_words;
!   if (doubleword_aligned_p)
      info->stack_offset += info->stack_offset & 1;
  
    max_regs = MAX_ARGS_IN_REGISTERS - info->reg_offset;
*************** function_arg (const CUMULATIVE_ARGS *cum
*** 3311,3320 ****
        return gen_rtx_PARALLEL (mode, gen_rtvec (2, real, imag));
      }
  
!   if (info.fpr_p)
!     return gen_rtx_REG (mode, FP_ARG_FIRST + info.reg_offset);
!   else
      return gen_rtx_REG (mode, GP_ARG_FIRST + info.reg_offset);
  }
  
  
--- 3299,3312 ----
        return gen_rtx_PARALLEL (mode, gen_rtvec (2, real, imag));
      }
  
!   if (!info.fpr_p)
      return gen_rtx_REG (mode, GP_ARG_FIRST + info.reg_offset);
+   else if (info.reg_offset == 1)
+     /* This code handles the special o32 case in which the second word
+        of the argument structure is passed in floating-point registers.  */
+     return gen_rtx_REG (mode, FP_ARG_FIRST + FP_INC);
+   else
+     return gen_rtx_REG (mode, FP_ARG_FIRST + info.reg_offset);
  }
  
  
*** /dev/null	Fri Apr 23 00:21:55 2004
--- testsuite/gcc.c-torture/execute/va-arg-26.c	Wed Sep 22 07:21:42 2004
***************
*** 0 ****
--- 1,20 ----
+ #include <stdarg.h>
+ 
+ double f (float f1, float f2, float f3, float f4,
+ 	  float f5, float f6, ...)
+ {
+   va_list ap;
+   double d;
+ 
+   va_start (ap, f6);
+   d = va_arg (ap, double);
+   va_end (ap);
+   return d;
+ }
+ 
+ int main ()
+ {
+   if (f (1, 2, 3, 4, 5, 6, 7.0) != 7.0)
+     abort ();
+   exit (0);
+ }