This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Support for MIPS r5900
- From: Richard Sandiford <rdsandiford at googlemail dot com>
- To: JÃrgen Urban <JuergenUrban at gmx dot de>
- Cc: gcc-patches at gcc dot gnu dot org
- Date: Mon, 07 Jan 2013 21:52:48 +0000
- Subject: Re: Support for MIPS r5900
- References: <20130106225645.190700@gmx.net>
"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