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: Support for MIPS r5900


"JÃrgen Urban" <JuergenUrban@gmx.de> writes:
> ll, sc, dmult, ddiv, cvt.w.s, 64 bit FPU instructions.
> ll and sc is disabled with "-mno-llsc" and works.
> cvt.w.s is replaced by trunc.w.s. This seems to work.

Probably showing my ignorance, but I couldn't see this in the patch.

> I disabled 64 bit FPU instructions by "-msoft-float". This works, but
> using "-msingle-float" fails. This would be the better
> configuration. There are still 64 bit FPU instructions used (e.g. "dmfc1
> $2,$f0" when using "long double" multiplication). So "-msingle-float"
> doesn't seem to work on generic mips64-linux-gnu.

Right.  That combination hasn't really been defined.  What happens
for plain doubles?  Do you pass those in FPRs or GPRs?

> I tried to disable dmult and ddiv (see mips.md). Disabling worked, but
> now muldi3 calls itself in libgcc2. I thought this should work, because
> I got this working with GCC 4.3, but the latest GCC version is a
> problem. multi3 is calling muldi3, so that muldi3 should be able to use
> mulsi3, because it is the same C code in libgcc2. Can someone get me
> some hints or comments? How can this be debugged?

Not sure, sorry.

> Does someone know how to enable TImode in MIPS ABI o32 (this doesn't
> need to use the 128 bit instructions at the moment)? There is some old
> code for the Playstation 2 which needs this. I know that TImode is
> supported in ABI n32, but some code uses also the 32 bit FPU and FPU
> registers are not available with "-msoft-float" in inline assembler.

The n32 TImode support you mention uses pairs of GPRs, whereas I imagine
you'd eventually want to use a single 128-bit GPR.  Is that right?

TImode in the current n32 sense doesn't really make practical sense
for 32-bit targets.  We'd be dealing with quad registers in that case.
I think it would only make sense with TImode registers.

ISTR the TImode registers being a can of worms, especially with the
optimisation to only store the lower 64 bits if the upper 64 weren't used.
(Am I remembering that right?)

When you submit the TImode register support, please make the support
itself a separate patch from the ABI changes.  I.e. one patch to
add TImode registers, one to add TImode to o32, one to add single-GPR
TImode to n32, etc.  For the record, I think all those patches would be
too invasive this late into the 4.8 cycle so would have to wait for 4.9.

Also, any ABI changes should be conditional on a new flag rather than
keyed off the architecture.  That flag would then be the default for
your new configuration.

> What is the best way to change the alignment to 128 bit for all
> structures and stack in any MIPS ABI? Much old code for the Playstation
> 2 expects this.

The stack is STACK_BOUNDARY (already 128 for n32).  Do you mean the
padding of all structure types, or just global structure variables?
If the former, it sounds like ROUND_TYPE_ALIGN, but also sounds scary :-)
If the latter, it's DATA_ALIGNMENT.

> @@ -15801,6 +15816,11 @@ mips_reorg_process_insns (void)
>    if (TARGET_FIX_VR4120 || TARGET_FIX_24K)
>      cfun->machine->all_noreorder_p = false;
>  
> +  /* Code compiled for R5900 can't be all noreorder because
> +     we rely on the assembler to work around some errata.  */
> +  if (TARGET_MIPS5900)
> +    cfun->machine->all_noreorder_p = false;
> +
>    /* The same is true for -mfix-vr4130 if we might generate MFLO or
>       MFHI instructions.  Note that we avoid using MFLO and MFHI if
>       the VR4130 MACC and DMACC instructions are available instead;

Please fold this into the clause above it.

> +/* Target supports instructions dmult and dmultu (integer). */
> +#define TARGET_HAS_DMULT	(TARGET_64BIT				\
> +				 && !TARGET_MIPS5900)

Please use ISA_HAS_* for consistency with other macros.  I think it'd
be better to drop the '(integer)'.

> +/* Target supports instructions mult and multu in 32 bit mode (integer). */
> +#define TARGET_HAS_MULT		(mips_isa >= 1)

...and here drop 'in 32 bit mode (integer)'.  32-bit-mode isn't really
relevant here.

> +/* Target supports instructions dmult and dmultu (integer). */
> +#define TARGET_HAS_DDIV		(TARGET_64BIT				\
> +				 && !TARGET_MIPS5900)

Same as above.

> +/* Target supports instructions mult and multu in 32 bit mode (integer). */
> +#define TARGET_HAS_DIV		(mips_isa >= 1)

Here too, plus "div" rather than "mult".

> @@ -841,10 +859,10 @@ struct mips_cpu_info {
>  
>  /* ISA has the integer conditional move instructions introduced in mips4 and
>     ST Loongson 2E/2F.  */
> -#define ISA_HAS_CONDMOVE        (ISA_HAS_FP_CONDMOVE || TARGET_LOONGSON_2EF)
> +#define ISA_HAS_CONDMOVE        (ISA_HAS_FP_CONDMOVE || TARGET_LOONGSON_2EF || TARGET_MIPS5900)

GCC has a strict 80-column limit, so please make this:

#define ISA_HAS_CONDMOVE	(ISA_HAS_FP_CONDMOVE \
				 || TARGET_LOONGSON_2EF \
				 || TARGET_MIPS5900)

>  /* ISA has LDC1 and SDC1.  */
> -#define ISA_HAS_LDC1_SDC1	(!ISA_MIPS1 && !TARGET_MIPS16)
> +#define ISA_HAS_LDC1_SDC1	(!ISA_MIPS1 && !TARGET_MIPS16 && !TARGET_MIPS5900)

Same 3-line expansion here.

> @@ -974,7 +993,11 @@ struct mips_cpu_info {
>  /* True if trunc.w.s and trunc.w.d are real (not synthetic)
>     instructions.  Both require TARGET_HARD_FLOAT, and trunc.w.d
>     also requires TARGET_DOUBLE_FLOAT.  */
> -#define ISA_HAS_TRUNC_W		(!ISA_MIPS1)
> +#define ISA_HAS_TRUNC_W_D	(!ISA_MIPS1)
> +
> +/* True if trunc.w.s is real (not synthetic) instructions.
> +   Requires TARGET_HARD_FLOAT.  */
> +#define ISA_HAS_TRUNC_W_S	(ISA_HAS_TRUNC_W_D || TARGET_MIPS5900)

First comment still describes both cases, so I think the second one
is redundant.  Just:

/* True if trunc.w.s and trunc.w.d are real (not synthetic)
   instructions.  Both require TARGET_HARD_FLOAT, and trunc.w.d
   also requires TARGET_DOUBLE_FLOAT.  */
#define ISA_HAS_TRUNC_W_D	(!ISA_MIPS1)
#define ISA_HAS_TRUNC_W_S	(ISA_HAS_TRUNC_W_D || TARGET_MIPS5900)

> @@ -726,7 +727,7 @@
>  ;; This mode iterator allows :MOVECC to be used anywhere that a
>  ;; conditional-move-type condition is needed.
>  (define_mode_iterator MOVECC [SI (DI "TARGET_64BIT")
> -                              (CC "TARGET_HARD_FLOAT && !TARGET_LOONGSON_2EF")])
> +                              (CC "TARGET_HARD_FLOAT && !TARGET_LOONGSON_2EF && !TARGET_MIPS5900")])

Same three-line expansion here:

(define_mode_iterator MOVECC [SI (DI "TARGET_64BIT")
			      (CC "TARGET_HARD_FLOAT
				   && !TARGET_LOONGSON_2EF
				   && !TARGET_MIPS5900")])

> @@ -1900,7 +1901,7 @@
>    [(set (match_operand:DI 0 "muldiv_target_operand" "=ka")
>  	(mult:DI (any_extend:DI (match_operand:SI 1 "register_operand" "d"))
>  		 (any_extend:DI (match_operand:SI 2 "register_operand" "d"))))]
> -  "!TARGET_64BIT && (!TARGET_FIX_R4000 || ISA_HAS_DSP)"
> +  "(!TARGET_64BIT || (TARGET_64BIT && !TARGET_HAS_DMULT)) && (!TARGET_FIX_R4000 || ISA_HAS_DSP)"
>  {
>    if (ISA_HAS_DSP_MULT)
>      return "mult<u>\t%q0,%1,%2";

Just:

  "!ISA_HAS_DMULTU && (!TARGET_FIX_R4000 || ISA_HAS_DSP)"

Please update <u>mulsidi3_32bit_mips16 and <u>mulsidi3_32bit_r4000
in the same way.

> @@ -1927,7 +1928,7 @@
>  		 (any_extend:DI (match_operand:SI 2 "register_operand" "d"))))
>     (clobber (match_scratch:TI 3 "=x"))
>     (clobber (match_scratch:DI 4 "=d"))]
> -  "TARGET_64BIT && !TARGET_FIX_R4000 && !ISA_HAS_DMUL3 && !TARGET_MIPS16"
> +  "TARGET_64BIT && !TARGET_FIX_R4000 && !ISA_HAS_DMUL3 && !TARGET_MIPS16 && TARGET_HAS_DMULT"
>    "#"
>    "&& reload_completed"
>    [(const_int 0)]

Just:

  "ISA_HAS_DMULTU && !TARGET_FIX_R4000 && !ISA_HAS_DMUL3 && !TARGET_MIPS16"

Please update <u>mulsidi3_64bit_mips16 in the same way.

> @@ -2105,7 +2106,7 @@
>  {
>    rtx hilo;
>  
> -  if (TARGET_64BIT)
> +  if (TARGET_64BIT && TARGET_HAS_DMULT)
>      {
>        hilo = gen_rtx_REG (TImode, MD_REG_FIRST);
>        emit_insn (gen_<u>mulsidi3_64bit_hilo (hilo, operands[1], operands[2]));

Here too just ISA_HAS_DMULT.  Several other cases later on, I won't
bore you with them all :-)

> @@ -2537,7 +2541,7 @@
>     (set (match_operand:GPR 3 "register_operand")
>  	(mod:GPR (match_dup 1)
>  		 (match_dup 2)))]
> -  "!TARGET_FIX_VR4120"
> +  "!TARGET_FIX_VR4120 && TARGET_HAS_<D>DIV"
>  {
>    if (TARGET_MIPS16)
>      {

Would prefer the ISA_HAS_<D>DIV first.  Please update the MIPS16 patterns
in the same way.

> @@ -1881,11 +1881,17 @@ mipsisa64sb1-*-elf* | mipsisa64sb1el-*-e
>  	target_cpu_default="MASK_64BIT|MASK_FLOAT64"
>  	tm_defines="${tm_defines} MIPS_ISA_DEFAULT=64 MIPS_CPU_STRING_DEFAULT=\\\"sb1\\\" MIPS_ABI_DEFAULT=ABI_O64"
>  	;;
> -mips-*-elf* | mipsel-*-elf*)
> +mips-*-elf* | mipsel-*-elf* | mipsr5900-*-elf* | mipsr5900el-*-elf*)
>  	tm_file="elfos.h newlib-stdint.h ${tm_file} mips/elf.h"
>  	tmake_file="mips/t-elf"
>  	;;
> -mips64-*-elf* | mips64el-*-elf*)
> +mips64r5900-*-elf* | mips64r5900el-*-elf*)
> +	tm_file="elfos.h newlib-stdint.h ${tm_file} mips/elf.h"
> +	tmake_file="mips/t-elf"
> +	target_cpu_default="MASK_64BIT"
> +	tm_defines="${tm_defines} MIPS_ISA_DEFAULT=3 MIPS_ABI_DEFAULT=ABI_N32"
> +	;;
> +mips64-*-elf* | mips64el-*-elf* | mips64r5900-*-elf* | mips64r5900el-*-elf*)
>  	tm_file="elfos.h newlib-stdint.h ${tm_file} mips/elf.h"
>  	tmake_file="mips/t-elf"
>  	target_cpu_default="MASK_64BIT|MASK_FLOAT64"

The change to the "mips64-*-elf* | mips64el-*-elf*)" line looks unnecessary.

> @@ -3374,7 +3400,7 @@ case "${target}" in
>  		supported_defaults="abi arch arch_32 arch_64 float tune tune_32 tune_64 divide llsc mips-plt synci"
>  
>  		case ${with_float} in
> -		"" | soft | hard)
> +		"" | soft | hard | single | double)
>  			# OK
>  			;;
>  		*)

Please leave this out for now and add it with the ABI changes mentioned
above.

I can't approve the libgcc bits, but I'm afraid they probably tip the
balance against this being acceptable for 4.8.

Richard


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