Matthew Fortune <matthew.fort...@imgtec.com> writes:
> *) Dwarf debug for 64-bit values in floating point values for FPXX can't
> be strictly correct for both 32-bit and 64-bit registers but opts to
> describe one 64-bit register as that is what the FPXX ABI is emulating.
> I have not yet checked what exactly happens in GDB when confronted with
> this and 32-bit registers. This also impacts frame information described
> via mips_save_reg and mips_restore_reg. Advice on this would be
> appreciated.
I'm not sure what's best either. Clearly it's something that needs
to be spelled out in the ABI, but I can imagine it would be dictated
by what consumers like the unwinder and gdb find easiest to handle.
> *) ISA_HAS_MXHC1 could be defined as true for all three O32 FP ABIs but
> I left out FP32 to maintain historic behaviour. It should be safe to
> Include it though. Thoughts?
Sounds like the right call to me FWIW. Enabling it for FP32 is a separate
change really.
> *) Because GCC can be built to have mfpxx or mfp64 as the default option
> the ASM_SPEC has to handle these specially such that they are not
> passed in conjunction with -msingle-float. Depending on how all this
> option handling pans out then this may also need to account for
> msoft-float as well. It is an error to have -msoft-float and -mfp64 in
> the assembler.
The assembler and GCC shouldn't treat the options differently though.
Either it should be OK for both or neither.
> @@ -5141,7 +5141,7 @@ mips_get_arg_info (struct mips_arg_info *info, const
> CUMULATIVE_ARGS *cum,
> || SCALAR_FLOAT_TYPE_P (type)
> || VECTOR_FLOAT_TYPE_P (type))
> && (GET_MODE_CLASS (mode) == MODE_FLOAT
> - || mode == V2SFmode)
> + || (TARGET_PAIRED_SINGLE_FLOAT && mode == V2SFmode))
> && GET_MODE_SIZE (mode) <= UNITS_PER_FPVALUE);
> break;
This looks odd. We shouldn't have V2SF values if there's no ISA support
for them.
> @@ -5636,7 +5636,7 @@ mips_return_fpr_pair (enum machine_mode mode,
> {
> int inc;
>
> - inc = (TARGET_NEWABI ? 2 : MAX_FPRS_PER_FMT);
> + inc = ((TARGET_NEWABI || mips_abi == ABI_32) ? 2 : MAX_FPRS_PER_FMT);
Formatting nit: no extra brackets here.
> @@ -6508,13 +6508,27 @@ mips_output_64bit_xfer (char direction, unsigned int
> gpreg, unsigned int fpreg)
> if (TARGET_64BIT)
> fprintf (asm_out_file, "\tdm%cc1\t%s,%s\n", direction,
> reg_names[gpreg], reg_names[fpreg]);
> - else if (TARGET_FLOAT64)
> + else if (ISA_HAS_MXHC1)
> {
> fprintf (asm_out_file, "\tm%cc1\t%s,%s\n", direction,
> reg_names[gpreg + TARGET_BIG_ENDIAN], reg_names[fpreg]);
> fprintf (asm_out_file, "\tm%chc1\t%s,%s\n", direction,
> reg_names[gpreg + TARGET_LITTLE_ENDIAN], reg_names[fpreg]);
> }
> + else if (TARGET_FLOATXX && direction == 't')
> + {
> + /* Use the argument save area to move via memory. */
> + fprintf (asm_out_file, "\tsw\t%s,0($sp)\n", reg_names[gpreg]);
> + fprintf (asm_out_file, "\tsw\t%s,4($sp)\n", reg_names[gpreg + 1]);
> + fprintf (asm_out_file, "\tldc1\t%s,0($sp)\n", reg_names[fpreg]);
> + }
> + else if (TARGET_FLOATXX && direction == 'f')
> + {
> + /* Use the argument save area to move via memory. */
> + fprintf (asm_out_file, "\tsdc1\t%s,0($sp)\n", reg_names[fpreg]);
> + fprintf (asm_out_file, "\tlw\t%s,0($sp)\n", reg_names[gpreg]);
> + fprintf (asm_out_file, "\tlw\t%s,4($sp)\n", reg_names[gpreg + 1]);
> + }
The argument save area might be in use. E.g. if an argument register
gets spilled, we'll generally try to spill it to the save area rather
than create a new stack slot for it.
This case should always be handled via SECONDARY_MEMORY_NEEDED.
> @@ -10499,7 +10544,7 @@ mips_for_each_saved_acc (HOST_WIDE_INT sp_offset,
> mips_save_restore_fn fn)
> static void
> mips_save_reg (rtx reg, rtx mem)
> {
> - if (GET_MODE (reg) == DFmode && !TARGET_FLOAT64)
> + if (GET_MODE (reg) == DFmode && !TARGET_FLOAT64 && !TARGET_FLOATXX)
TARGET_FLOAT32, and elsewhere.
> @@ -12202,7 +12247,8 @@ mips_secondary_reload_class (enum reg_class rclass,
> return NO_REGS;
>
> /* Otherwise, we need to reload through an integer register. */
> - return GR_REGS;
> + if (regno >= 0)
> + return GR_REGS;
> }
> if (FP_REG_P (regno))
> return reg_class_subset_p (rclass, GR_REGS) ? NO_REGS : GR_REGS;
Why's this change needed? Although I assume it's dead code if you tested
against LRA.
> @@ -12210,6 +12256,22 @@ mips_secondary_reload_class (enum reg_class rclass,
> return NO_REGS;
> }
>
> +/* Implement HARD_REGNO_CALLER_SAVE_MODE.
> + Always save floating-point registers using their current mode to avoid
> + using a 64-bit load/store when a 64-bit FP register only contains a 32-bit
> + mode. */
> +
> +enum machine_mode
> +mips_hard_regno_caller_save_mode (unsigned int regno, unsigned int nregs,
> + enum machine_mode mode)
> +{
> + return ((FP_REG_P (regno) && (mode) != VOIDmode
> + && (unsigned) hard_regno_nregs[regno][mode] == nregs
> + && HARD_REGNO_MODE_OK (regno, mode))
> + ? mode
> + : choose_hard_reg_mode (regno, nregs, false));
> +}
I'd rather go with Chao-Ying's patch:
https://gcc.gnu.org/ml/gcc/2013-01/msg00180.html
if there are no known downsides.
> @@ -17041,6 +17103,14 @@ mips_option_override (void)
> target_flags &= ~MASK_FLOAT64;
> }
>
> + if (mips_abi != ABI_32 && TARGET_FLOATXX)
> + error ("%<-mfpxx%> can only be used with the o32 ABI");
> + else if (ISA_MIPS1
> + && (TARGET_FLOATXX
> + || TARGET_FLOAT64))
> + error ("%<-march=%s%> requires %<-mfp32%>",
> + mips_arch_info->name);
!TARGET_FLOAT32. No need for the line split.
> @@ -1737,6 +1753,15 @@ struct mips_cpu_info {
>
> #define MODES_TIEABLE_P mips_modes_tieable_p
>
> +/* Odd numbered single precision registers are not considered call saved
> + for O32 FPXX as they will be clobbered when run on an FR=1 FPU. */
Nit: Odd-numbered, single-precision
> +#define HARD_REGNO_CALL_PART_CLOBBERED(REGNO, MODE) \
> + (TARGET_FLOATXX && ((MODE) == SFmode || (MODE) == SImode) \
> + && FP_REG_P (REGNO) && (REGNO & 1))
hard_regno_nregs[REGNO][MODE] == 1 or GET_MODE_SIZE (MODE) <= 4
might be better than explicit mode checks.
> @@ -2069,6 +2094,16 @@ enum reg_class
> #define SECONDARY_OUTPUT_RELOAD_CLASS(CLASS, MODE, X)
> \
> mips_secondary_reload_class (CLASS, MODE, X, false)
>
> +/* When targetting the O32 FPXX ABI then all doubleword or greater moves
> + to/from FP registers must be performed by FR mode aware instructions.
FR-mode-aware
> diff --git a/gcc/config/mips/mips.md b/gcc/config/mips/mips.md
> index f914ab6..9acc565 100644
> --- a/gcc/config/mips/mips.md
> +++ b/gcc/config/mips/mips.md
> @@ -431,9 +431,12 @@
> (const_string "none"))
>
> (define_attr "enabled" "no,yes"
> - (if_then_else (ior (eq_attr "compression" "all,none")
> - (and (eq_attr "compression" "micromips")
> - (match_test "TARGET_MICROMIPS")))
> + (if_then_else (and (not (and (eq_attr "move_type" "mtc,mfc")
> + (match_test "TARGET_FLOATXX && !ISA_HAS_MXHC1")
> + (eq_attr "dword_mode" "yes")))
> + (ior (eq_attr "compression" "all,none")
> + (and (eq_attr "compression" "micromips")
> + (match_test "TARGET_MICROMIPS"))))
> (const_string "yes")
> (const_string "no")))
Probably time for this to become a (cond [...]). Took me a couple of
goes to read it properly :-)
> diff --git a/gcc/testsuite/gcc.target/mips/call-clobbered-2.c
> b/gcc/testsuite/gcc.target/mips/call-clobbered-2.c
> new file mode 100644
> index 0000000..f47cdda
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/mips/call-clobbered-2.c
> @@ -0,0 +1,21 @@
> +/* Check that we handle call-clobbered FPRs correctly. */
> +/* { dg-skip-if "code quality test" { *-*-* } { "-O0" } { "" } } */
> +/* { dg-options "-mabi=32 -modd-spreg -mfp32 -ffixed-f0 -ffixed-f1
> -ffixed-f2 -ffixed-f3 -ffixed-f4 -ffixed-f5 -ffixed-f6 -ffixed-f7 -ffixed-f8
> -ffixed-f9 -ffixed-f10 -ffixed-f11 -ffixed-f12 -ffixed-f13 -ffixed-f14
> -ffixed-f15 -ffixed-f16 -ffixed-f17 -ffixed-f18 -ffixed-f19 -ffixed-f20
> -ffixed-f22 -ffixed-f24 -ffixed-f26 -ffixed-f28 -ffixed-f30" } */
> +
> +void bar (void);
> +float a;
> +float
> +foo ()
> +{
> + float b = a + 1.0f;
> + bar();
> + return b;
> +}
> +/* { dg-final { scan-assembler-times "lwc1" 2 } } */
> +/* { dg-final { scan-assembler-not "swc1" } } */
> +/* { dg-final { scan-assembler-times "sdc1" 2 } } */
> +/* { dg-final { scan-assembler-not "mtc" } } */
> +/* { dg-final { scan-assembler-not "mfc" } } */
> +/* { dg-final { scan-assembler-not "mthc" } } */
> +/* { dg-final { scan-assembler-not "mfhc" } } */
Why require sdc1 here? Would Chao-Ying's patch make this use SWC1 and LWC1
exclusively?
> diff --git a/gcc/testsuite/gcc.target/mips/call-clobbered-3.c
> b/gcc/testsuite/gcc.target/mips/call-clobbered-3.c
> new file mode 100644
> index 0000000..3b30ce6
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/mips/call-clobbered-3.c
> @@ -0,0 +1,21 @@
> +/* Check that we handle call-clobbered FPRs correctly. */
> +/* { dg-skip-if "code quality test" { *-*-* } { "-O0" "-Os" } { "" } } */
> +/* { dg-options "-mabi=32 -modd-spreg -mfpxx -ffixed-f0 -ffixed-f1
> -ffixed-f2 -ffixed-f3 -ffixed-f4 -ffixed-f5 -ffixed-f6 -ffixed-f7 -ffixed-f8
> -ffixed-f9 -ffixed-f10 -ffixed-f11 -ffixed-f12 -ffixed-f13 -ffixed-f14
> -ffixed-f15 -ffixed-f16 -ffixed-f17 -ffixed-f18 -ffixed-f19 -ffixed-f20
> -ffixed-f22 -ffixed-f24 -ffixed-f26 -ffixed-f28 -ffixed-f30" } */
> +
> +void bar (void);
> +float a;
> +float
> +foo ()
> +{
> + float b = a + 1.0f;
> + bar();
> + return b;
> +}
> +/* { dg-final { scan-assembler-times "lwc1" 3 } } */
> +/* { dg-final { scan-assembler-times "swc1" 1 } } */
> +/* { dg-final { scan-assembler-times "sdc1" 2 } } */
> +/* { dg-final { scan-assembler-not "mtc" } } */
> +/* { dg-final { scan-assembler-not "mfc" } } */
> +/* { dg-final { scan-assembler-not "mthc" } } */
> +/* { dg-final { scan-assembler-not "mfhc" } } */
Does there need to be an ldc1 as well? Please explain the skips in more
detail. "code quality test" only applies for -O0 skips. Anything skipped
on -Os or restricted to -Os should be justified.
> diff --git a/gcc/testsuite/gcc.target/mips/call-clobbered-4.c
> b/gcc/testsuite/gcc.target/mips/call-clobbered-4.c
> new file mode 100644
> index 0000000..872fd5e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/mips/call-clobbered-4.c
> @@ -0,0 +1,20 @@
> +/* Check that we handle call-clobbered FPRs correctly. */
> +/* { dg-skip-if "code quality test" { *-*-* } { "*" } { "-Os" } } */
> +/* { dg-options "-mabi=32 -modd-spreg -mfpxx -ffixed-f0 -ffixed-f1
> -ffixed-f2 -ffixed-f3 -ffixed-f4 -ffixed-f5 -ffixed-f6 -ffixed-f7 -ffixed-f8
> -ffixed-f9 -ffixed-f10 -ffixed-f11 -ffixed-f12 -ffixed-f13 -ffixed-f14
> -ffixed-f15 -ffixed-f16 -ffixed-f17 -ffixed-f18 -ffixed-f19 -ffixed-f20
> -ffixed-f22 -ffixed-f24 -ffixed-f26 -ffixed-f28 -ffixed-f30" } */
> +
> +void bar (void);
> +float a;
> +float
> +foo ()
> +{
> + float b = a + 1.0f;
> + bar();
> + return b;
> +}
> +/* { dg-final { scan-assembler-times "lwc1" 2 } } */
> +/* { dg-final { scan-assembler-times "sdc1" 2 } } */
> +/* { dg-final { scan-assembler-times "mtc" 1 } } */
> +/* { dg-final { scan-assembler-times "mfc" 1 } } */
> +/* { dg-final { scan-assembler-not "mthc" } } */
> +/* { dg-final { scan-assembler-not "mfhc" } } */
What's the difference between this and call-clobbered-3.c?
> @@ -248,6 +248,15 @@ set mips_option_groups {
> dump "-fdump-.*"
> }
>
> +foreach option {
> + 0 1 2 3 4 5 6 7 8 9
> + 10 11 12 13 14 15 16 17 18 19
> + 20 21 22 23 24 25 26 27 28 29
> + 30 31
> +} {
for { set option 0 } { $option < 32 } { incr option } {
> @@ -1083,7 +1096,8 @@ proc mips-dg-options { args } {
> # (*) needed by both -mbranch-likely and -mfix-r10000
> } elseif { $isa < 2
> && ([mips_have_test_option_p options "-mbranch-likely"]
> - || [mips_have_test_option_p options "-mfix-r10000"]) } {
> + || [mips_have_test_option_p options "-mfix-r10000"]
> + || ($gp_size == 32 && [mips_have_test_option_p options
> "-mfpxx"])) } {
Long line
> diff --git a/gcc/testsuite/gcc.target/mips/movdf-1.c
> b/gcc/testsuite/gcc.target/mips/movdf-1.c
> new file mode 100644
> index 0000000..d08b616
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/mips/movdf-1.c
> @@ -0,0 +1,13 @@
> +/* Check that we move DFmode values via memory between FP and GP. */
> +/* { dg-options "-mabi=32 -mfpxx isa=2" } */
> +
> +void bar (void);
> +
> +double
> +foo (int x, double a)
> +{
> + return a;
> +}
> +/* { dg-final { scan-assembler-not "mthc1" } } */
> +/* { dg-final { scan-assembler-not "mtc1" } } */
Probably worth adding some positive tests too, to make sure that the load
happens with ldc1, etc.
> +#elif __mips_fpr == 0
> +#define MOVE_DF_BYTE0t sw $4, 0($29); sw $5, 4($29); ldc1 $f12, 0($29)
> +#define MOVE_DF_BYTE0f sdc1 $f12, 0($29); lw $4, 0($29); lw $5, 4($29)
> +#define MOVE_DF_BYTE0(D) MOVE_DF_BYTE0##D
> +#define MOVE_DF_BYTE8t sw $6, 8($29); sw $7, 12($29); ldc1 $f14, 8($29)
> +#define MOVE_DF_BYTE8f sdc1 $f14, 8($29); lw $6, 8($29); lw $7, 12($29)
> +#define MOVE_DF_BYTE8(D) MOVE_DF_BYTE8##D
> +#define MOVE_DF_RETt(T) sw $2, 0($29); sw $3, 4($29); DELAYt (T, ldc1 $f0,
> 0($29))
> +#define MOVE_DF_RETf(T) sdc1 $f0, 0($29); lw $2, 0($29); DELAYf (T, lw $3,
> 4($29))
> +#define MOVE_DF_RET(D, T) MOVE_DF_RET##D(T)
> +#define MOVE_DC_RETt(T) sw $4, 8($29); sw $5, 12($29); ldc1 $f2, 8($29);
> MOVE_DF_RETt(T)
> +#define MOVE_DC_RETf(T) sdc1 $f2, 8($29); lw $4, 8($29); lw $5, 12($29);
> MOVE_DF_RETf(T)
> +#define MOVE_DC_RET(D, T) MOVE_DF_RET##D(T)
> #elif defined(__MIPSEB__)
> /* FPRs are little-endian. */
> #define MOVE_DF_BYTE0(D) m##D##c1 $4,$f13; m##D##c1 $5,$f12
Is this always safe? CALL_STUB_RET in particular uses a special ABI
rather than the standard one, so I don't think we can assume that
it's safe to clobber the parameter save area.
Thanks,
Richard
