Re: [patch] MIPS: 64bit floating point support for MIPS32R2

[Richard Sandiford <richard at codesourcery dot com>]

David Ung <davidu@mips.com> writes:
> MIPS32 rev 2 optionally supports 64bit floating unit.  This patch
> allows the use of 64bit floats by passing the -mfp64 option when used
> along with a MIPS32R2 cpu (the 24k/34k family with float point all
> have 64bit floating point units).  Since the default o32 abi assume
> 32bit float with 32bit integer unit, it slightly modifies it that the
> float parameters $f12/$14 etc are 64bit.

I think we should document exactly what effect -mfp64 has on the 32-bit
ABIs, as it's quite a big change to have 32-bit conventions on the GPR
side and 64-bit conventions on the FPR side.  The change above might be
the only one that applies to o32, but the 32-bit EABI is also affected.

We should also explicitly state that the full 64 bits are caller-saved,
and that the caller-saved registers are the same as for -mfp32, assuming
both statements are true.  (I realise this might seen nitpicky.  The reason
I mention it is that the n32 and n64 ABIs chose to use a different set of
call-saved registers, both from each other and from the o32 ABI.)

> +;; mips32r2 optionally supports a full 64-bit fpu

Redundant comment.

> +(define_insn "*movdi_32bit_mips32r2"
> +  [(set (match_operand:DI 0 "nonimmediate_operand" 
> "=d,d,d,m,*a,*d,*B*C*D,*B*C*D,*d,*m,*f,*f,*f,*d,*m")
> +       (match_operand:DI 1 "move_operand" 
> "d,i,m,d,*J*d,*a,*d,*m,*B*C*D,*B*C*D,*f,*J*d,*m,*f,*f"))]
> +  "!TARGET_64BIT && TARGET_FLOAT64 && !TARGET_MIPS16
> +   && (register_operand (operands[0], DImode)
> +       || reg_or_0_operand (operands[1], DImode))"
> +  { return mips_output_move (operands[0], operands[1]); }
> +  [(set_attr "type" 
> "arith,arith,load,store,mthilo,mfhilo,xfer,load,xfer,store,fmove,xfer,fpload,xfer,fpstore")
> +   (set_attr "mode"    "DI")
> +   (set_attr "length"   "8,16,*,*,8,8,8,*,8,*,4,8,*,8,*")])

I'm suspicious about these d <-> f alternatives for reasons explained below.

> @@ -3802,7 +3814,7 @@
>   (define_insn "*movdf_hardfloat_64bit"
>     [(set (match_operand:DF 0 "nonimmediate_operand" "=f,f,f,m,m,*f,*d,*d,*d,*m")
>          (match_operand:DF 1 "move_operand" "f,G,m,f,G,*d,*f,*d*G,*m,*d"))]
> -  "TARGET_HARD_FLOAT && TARGET_DOUBLE_FLOAT && TARGET_64BIT
> +  "TARGET_HARD_FLOAT && TARGET_DOUBLE_FLOAT && TARGET_FLOAT64
>      && (register_operand (operands[0], DFmode)
>          || reg_or_0_operand (operands[1], DFmode))"
>     { return mips_output_move (operands[0], operands[1]); }
> @@ -3813,7 +3825,7 @@
>   (define_insn "*movdf_hardfloat_32bit"
>     [(set (match_operand:DF 0 "nonimmediate_operand" "=f,f,f,m,m,*f,*d,*d,*d,*m")
>          (match_operand:DF 1 "move_operand" "f,G,m,f,G,*d,*f,*d*G,*m,*d"))]
> -  "TARGET_HARD_FLOAT && TARGET_DOUBLE_FLOAT && !TARGET_64BIT
> +  "TARGET_HARD_FLOAT && TARGET_DOUBLE_FLOAT && !TARGET_FLOAT64
>      && (register_operand (operands[0], DFmode)
>          || reg_or_0_operand (operands[1], DFmode))"
>     { return mips_output_move (operands[0], operands[1]); }

The lengths will not be right for the d <-> f cases.  I think you need
a new pattern for DFmode, just as you did for DImode.

> @@ -3951,9 +3963,9 @@
>
>   ;; Load the low word of operand 0 with operand 1.
>   (define_insn "load_df_low"
> -  [(set (match_operand:DF 0 "register_operand" "=f,f")
> -       (unspec:DF [(match_operand:SI 1 "general_operand" "dJ,m")]
> -                  UNSPEC_LOAD_DF_LOW))]
> +  [(set (match_operand 0 "register_operand" "=f,f")
> +       (unspec [(match_operand:SI 1 "general_operand" "dJ,m")]
> +               UNSPEC_LOAD_DF_LOW))]
>     "TARGET_HARD_FLOAT && TARGET_DOUBLE_FLOAT && !TARGET_64BIT"
>   {
>     operands[0] = mips_subword (operands[0], 0);
> @@ -3965,10 +3977,10 @@
>   ;; Load the high word of operand 0 from operand 1, preserving the value
>   ;; in the low word.
>   (define_insn "load_df_high"
> -  [(set (match_operand:DF 0 "register_operand" "=f,f")
> -       (unspec:DF [(match_operand:SI 1 "general_operand" "dJ,m")
> -                   (match_operand:DF 2 "register_operand" "0,0")]
> -                  UNSPEC_LOAD_DF_HIGH))]
> +  [(set (match_operand 0 "register_operand" "=f,f")
> +       (unspec [(match_operand:SI 1 "general_operand" "dJ,m")
> +               (match_operand:DF 2 "register_operand" "0,0")]
> +               UNSPEC_LOAD_DF_HIGH))]
>     "TARGET_HARD_FLOAT && TARGET_DOUBLE_FLOAT && !TARGET_64BIT"
>   {
>     operands[0] = mips_subword (operands[0], 1);

You shouldn't really remove the modes here.  For one thing, it will
cause gen_load_df_high() to create noncanonical rtl like:

    (set (reg:DF ...) (unspec:VOID ...))

It also isn't really a good idea to have modeless destination match_operands.

You've kept the DFmode on the source operand in the load_df_high,
so I don't see how this pattern would ever have matched a DImode access
during your tests.  I think we need some gcc.target/mips testcases that
are known to exercise the new DImode versions of these patterns.

I also don't understand how these load_df* and store_df* patterns
generate the right code when moving between 2 32-bit GPRs and one
64-bit FPR, which is one the things your move pattern above seems
to use them for.  You allow 64-bit d <-> f moves but don't add any
support for m[tf]hc1.

> @@ -7632,6 +7632,8 @@
>     if (MIN (GET_MODE_SIZE (from), GET_MODE_SIZE (to)) <= UNITS_PER_WORD
>         && MAX (GET_MODE_SIZE (from), GET_MODE_SIZE (to)) > UNITS_PER_WORD)
>       {
> +      if (TARGET_FLOAT64)
> +       return reg_classes_intersect_p (FP_REGS, class);
>         if (TARGET_BIG_ENDIAN)
>          {
>            /* When a multi-word value is stored in paired floating-point

You don't explain this change.  It needs a comment.

> Index: gcc/gcc/gcc/config/mips/mips.h
> ===================================================================
> --- gcc.orig/gcc/gcc/config/mips/mips.h 2006-11-06 17:20:29.000000000 +0000
> +++ gcc/gcc/gcc/config/mips/mips.h      2006-11-06 17:39:00.000000000 +0000
> @@ -611,6 +611,7 @@
>      FP madd and msub instructions, and the FP recip and recip sqrt
>      instructions.  */
>   #define ISA_HAS_FP4             ((ISA_MIPS4                            \
> +                                 || (ISA_MIPS32R2 && TARGET_FLOAT64)   \
>                                    || ISA_MIPS64)                        \
>                                   && !TARGET_MIPS16)

This looks odd.  The macro controls things like madd.<fmt> and
recip.<fmt>.  Are those insns really only available if TARGET_FLOAT64?
V2.50 of the MIPS32r2 architecture manual suggests that the .s and .d
forms are available even in "16 FP registers mode"; the only exception
given is .ps, which we would never use unless TARGET_FLOAT64 anyway.

> @@ -840,6 +841,7 @@
>   %(subtarget_asm_debugging_spec) \
>   %{mabi=*} %{!mabi*: %(asm_abi_default_spec)} \
>   %{mgp32} %{mgp64} %{march=*} %{mxgot:-xgot} \
> +%{mfp32} %{mfp64} \
>   %{mshared} %{mno-shared} \
>   %{msym32} %{mno-sym32} \
>   %{mtune=*} %{v} \

This part is OK.

Richard

PS. Could you try attaching the patch as text/plain attachment?
Like you said privately, your mailer is changing whitespace and
wrapping lines.