This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


David Ung <davidu@mips.com> writes:
> New patch revision attached.

Thanks.  The patch is looking much better.

> This patch adds -mfp64 for MIPS32R2 to allow the use of 64bit float
> insn, mfhc1/mthc1 instructions to access the high half of the FPU
> register.  I've restricted the -mgp32 -mfp64 to O32 only.

Is that because you don't want to specify the changes to the 32-bit
EABI at this stage, or because there are known interaction problems?
(Either's fine, I'm just curious.)

> I've taken Richard's suggestion of retaining the modes in
> load_df_{low/high}.  As a consequence, I had to force the mode change
> (DI to DF) in mips_split_64bit_move such that the patterns would
> match.  (Instead of creating multiple versions of load_di_{low/high}
> and m{f/t}hc1_{di/df}) Some problems with complex float arguments etc
> which will be addressed in future patches.

And:

> Re-tested on cross-compiler (mipsisa32-sde-elf, O32 abi) under GNU sim
> with flags "-mip32r2 -mfp64".  regressions mostly ok.  (39 failures
> when running "-mips32r2", 42 failures with "-mip32r2 -mfp64")
> Bootstrapped on linux.

Sorry to be a git, but I'd really rather the results were the same
for both multilibs before anything goes in (or at least for there
to be known reasons for the differences, if the differences aren't
directly related to the new option combination).

> +      rtx d = dest;
> +      if (ISA_HAS_MXHC1 && GET_MODE (d) == DImode)
> +	d = gen_rtx_REG (DFmode, REGNO (d));

These insns only accept DFmode, so I'd suggest replacing all three
lines with:

         dest = gen_lowpart (DFmode, dest);

> +      emit_insn (gen_load_df_low (copy_rtx (d), mips_subword (src, 0)));
> +      if (ISA_HAS_MXHC1 && GET_CODE (src) != MEM)

I don't like the !MEM_P here; it implies that SRC can be a MEM
if ISA_HAS_MXHC1.  (It can't, and the fallback would generate
wrong code anyway.)

> +	emit_insn (gen_mthc1 (copy_rtx (d), mips_subword (src, 1),
> +			      copy_rtx (d)));
> +      else
> +	emit_insn (gen_load_df_high (copy_rtx (d), mips_subword (src, 1),
> +				     copy_rtx (d)));

Too many copy_rtx()es.  In the orignal, we used an uncopied value for
one operand.

It would probably be more canonical to use DImode rather than DFmode
as the standard mode for the ISA_HAS_MXHC1 case, because subreg-like
punning to DImode is allowed in more cases than to DFmode.  You could
arrange that by turning load_<mode>_low into a mode macro that is
expanded for both DImode and DFmode and replacing the code above with
something like:

        if (ISA_HAS_MXHC1)
          {
            dest = gen_lowpart (DImode, dest);
            emit_insn (gen_load_di_low (dest, mips_subword (src, 0)));
            emit_insn (gen_mthc1 (copy_rtx (dest), mips_subword (src, 1),
                                  copy_rtx (dest)));
          }
        else
          {
            emit_insn (gen_load_df_low (copy_rtx (dest),
                                        mips_subword (src, 0)));
            emit_insn (gen_load_df_high (dest, mips_subword (src, 1),
                        		 copy_rtx (dest)));
          }

You'd need to change the mthc1 mode from DFmode to DImode too, of course.

I'm not going to ask you to do that unless you want to.  It's just
something to bear in mind.  I don't know if it's related to the
complex float problems you mentioned above (probably not).

>      }
>    else if (FP_REG_RTX_P (src))
>      {
>        /* Storing an FPR into memory or GPRs.  */
> -      emit_move_insn (mips_subword (dest, 0), mips_subword (src, 0));
> -      emit_insn (gen_store_df_high (mips_subword (dest, 1), src));
> +      rtx s = src;
> +      if (ISA_HAS_MXHC1 && GET_MODE (s) == DImode)
> +	s = gen_rtx_REG (DFmode, REGNO (s));

Likewise replace these three lines with:

        src = gen_lowpart (DFmode, src);

and...

> +      emit_move_insn (mips_subword (dest, 0), mips_subword (s, 0));
> +      if (ISA_HAS_MXHC1 && GET_CODE (dest) != MEM)

...remove this MEM check.  Again, DImode would probably be a more
canonical choice, and in this case wouldn't need any extra patterns,
just the mfhc1 mode change.

> @@ -4804,7 +4817,8 @@
>  	 only one right answer here.  */
>        if (TARGET_64BIT && TARGET_DOUBLE_FLOAT && !TARGET_FLOAT64)
>  	error ("unsupported combination: %s", "-mgp64 -mfp32 -mdouble-float");
> -      else if (!TARGET_64BIT && TARGET_FLOAT64)
> +      else if (!TARGET_64BIT && TARGET_FLOAT64 
> +	       && !(ISA_MIPS32R2 && mips_abi == ABI_32))
>  	error ("unsupported combination: %s", "-mgp32 -mfp64");
>        else if (TARGET_SINGLE_FLOAT && TARGET_FLOAT64)
>  	error ("unsupported combination: %s", "-mfp64 -msingle-float");

Minor nit, but I'd prefer ISA_HAS_MXHC1 to ISA_MIPS32R2 here.
It's the m[tf]hc1 insns that make the difference between sensible
-mfp64 -mgp32 support and none.  I realise the two are equivalent,
but ISA_HAS_MXHC1 makes the reason clearer.

I think we should also change the error message to:

        -mgp32 and -mfp64 can only be combined if the target supports the mfhc1 and mthc1 instructions

or something.  (I'm open to better suggestions.)  It's no longer true to
say that the option combination isn't supported at all.

> @@ -7639,6 +7653,10 @@
>    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)
> +	/* Don't allow implicit conversion to/from another class that
> +	   isn't float.  */
> +	return reg_classes_intersect_p (FP_REGS, class);
>        if (TARGET_BIG_ENDIAN)
>  	{
>  	  /* When a multi-word value is stored in paired floating-point

You still haven't explained why this change is needed.  It looks at first
glance like it might be working around a problem rather than fixing one.

> @@ -7669,6 +7687,7 @@
>        && GET_MODE_SIZE (to) >= UNITS_PER_WORD
>        && reg_classes_intersect_p (FP_REGS, class))
>      return true;
> +
>    return false;
>  }

If you're going to add whitespace here, please add it above the "if" too.

> +(define_insn "*movdi_32bit_mips32r2_fp64"
> +  [(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,*")])
> +

As things stand, the *B*C*D alternatives aren't needed, because we only
allow those registers to store word-sized values.  The cases wouldn't be
handled correctly if we ever did use them (for the same reason that FPR
moves weren't handled correctly in the first cut of the patch).  I think
we should just drop them.

Minor nit, but I think *movdi_gp32_fp64 would be a better name.

> @@ -3815,6 +3828,17 @@
>     (set_attr "mode"	"DF")
>     (set_attr "length"	"4,8,*,*,*,8,8,8,*,*")])
>  
> +(define_insn "*movdf_hardfloat_mips32r2_fp64"
> +  [(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_FLOAT64

Missing !TARGET_64BIT here.  Might as well drop TARGET_DOUBLE_FLOAT
for consistency with your other patterns; it will never be false if
TARGET_FLOAT64.  As above, I think *movvdf_hardfloat_gp32_fp64
would be a better name.  

> +   && (register_operand (operands[0], DFmode)
> +       || reg_or_0_operand (operands[1], DFmode))"
> +  { return mips_output_move (operands[0], operands[1]); }
> +  [(set_attr "type"	"fmove,xfer,fpload,fpstore,store,xfer,xfer,arith,load,store")
> +   (set_attr "mode"	"DF")
> +   (set_attr "length"	"4,4,*,*,*,8,8,8,*,*")])

I think the second length should be 8, because it splits into an
mtc1 and an mthc1, but correct me if I'm wrong.  Looks good otherwise.

> +;; Move operand 1 to the high word of operand 0 using mthc1, preserving the
> +;; value in the low word.
> +(define_insn "mthc1"
> +  [(set (match_operand:DF 0 "register_operand" "=f")
> +	(unspec:DF [(match_operand:SI 1 "general_operand" "dJ")
> +		    (match_operand:DF 2 "register_operand" "0")]
> +		    UNSPEC_MTHC1))]
> +  "TARGET_HARD_FLOAT && !TARGET_64BIT && ISA_HAS_MXHC1"
> +  "mthc1\t%z1,%0"
> +  [(set_attr "type"	"xfer")
> +   (set_attr "mode"	"SF")])
> +
> +;; Move high word of operand 1 to operand 0 using mfhc1.  The corresponding
> +;; low-word move is done in the normal way.
> +(define_insn "mfhc1"
> +  [(set (match_operand:SI 0 "register_operand" "=d")
> +	(unspec:SI [(match_operand:DF 1 "register_operand" "f")]
> +		    UNSPEC_MFHC1))]
> +  "TARGET_HARD_FLOAT && !TARGET_64BIT && ISA_HAS_MXHC1"
> +  "mfhc1\t%0,%1"
> +  [(set_attr "type"	"xfer")
> +   (set_attr "mode"	"SF")])

Looks good, thanks.

> +	} elseif {$flag == "-mfp64"} {
> +	    if {$mips_isa < 33 || $mips_float != "hard"} {
> +		set matches 0
> +	    }

Likewise.

The main missing thing I can see is the documentation changes
I mentioned before, including the effect that -mfp64 has on the ABI.
Otherwise, with the above fixed, and with the regressions from -mfp32
smoothed out, the patch looks good to go.

Richard


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]