This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH 1/2] Add support for O32 FPXX ABI
- From: Richard Sandiford <rdsandiford at googlemail dot com>
- To: Matthew Fortune <Matthew dot Fortune at imgtec dot com>
- Cc: "macro\ at codesourcery dot com" <macro at codesourcery dot com>, "Joseph Myers \(joseph\ at codesourcery dot com\)" <joseph at codesourcery dot com>, "binutils\ at sourceware dot org" <binutils at sourceware dot org>, Rich Fuhler <Rich dot Fuhler at imgtec dot com>
- Date: Wed, 07 May 2014 09:01:08 +0100
- Subject: Re: [PATCH 1/2] Add support for O32 FPXX ABI
- Authentication-results: sourceware.org; auth=none
- References: <6D39441BF12EF246A7ABCE6654B023534DAAAB at LEMAIL01 dot le dot imgtec dot org> <878urd37t7 dot fsf at sandifor-thinkpad dot stglab dot manchester dot uk dot ibm dot com> <6D39441BF12EF246A7ABCE6654B023534DFD9B at LEMAIL01 dot le dot imgtec dot org> <87fvll1ha9 dot fsf at sandifor-thinkpad dot stglab dot manchester dot uk dot ibm dot com> <6D39441BF12EF246A7ABCE6654B023534E07CB at LEMAIL01 dot le dot imgtec dot org> <87k3awnuf1 dot fsf at talisman dot default> <6D39441BF12EF246A7ABCE6654B023534E0F50 at LEMAIL01 dot le dot imgtec dot org> <87ppknzv7m dot fsf at talisman dot default> <6D39441BF12EF246A7ABCE6654B023534E16D7 at LEMAIL01 dot le dot imgtec dot org> <877g6vzpj1 dot fsf at talisman dot default> <6D39441BF12EF246A7ABCE6654B023534E3030 at LEMAIL01 dot le dot imgtec dot org> <87eh0yl4xx dot fsf at talisman dot default> <6D39441BF12EF246A7ABCE6654B023534E3AA8 at LEMAIL01 dot le dot imgtec dot org> <87d2ghk7fu dot fsf at talisman dot default> <6D39441BF12EF246A7ABCE6654B023535127C8 at LEMAIL01 dot le dot imgtec dot org> <87r44mve6s dot fsf at sandifor-thinkpad dot stglab dot manchester dot uk dot ibm dot com> <6D39441BF12EF246A7ABCE6654B02353512A10 at LEMAIL01 dot le dot imgtec dot org> <87r44leu34 dot fsf at talisman dot default> <6D39441BF12EF246A7ABCE6654B0235351D76A at LEMAIL01 dot le dot imgtec dot org> <87r446ww90 dot fsf at talisman dot default> <6D39441BF12EF246A7ABCE6654B02353521DF6 at LEMAIL01 dot le dot imgtec dot org>
Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
>> > + if (!mips_elf_tdata (obfd)->abiflags_valid)
>> > + {
>> > + /* Copy input abiflags if output abiflags are not already valid. */
>> > + mips_elf_tdata (obfd)->abiflags = mips_elf_tdata (ibfd)->abiflags;
>> > + mips_elf_tdata (obfd)->abiflags_valid = TRUE;
>> > + }
>> > +
>> > + if (! elf_flags_init (obfd))
>> > + {
>> > + elf_flags_init (obfd) = TRUE;
>> > + elf_elfheader (obfd)->e_flags = new_flags;
>> > + elf_elfheader (obfd)->e_ident[EI_CLASS]
>> > + = elf_elfheader (ibfd)->e_ident[EI_CLASS];
>> > +
>> > + if (bfd_get_arch (obfd) == bfd_get_arch (ibfd)
>> > + && (bfd_get_arch_info (obfd)->the_default
>> > + || mips_mach_extends_p (bfd_get_mach (obfd),
>> > + bfd_get_mach (ibfd))))
>> > + {
>> > + if (! bfd_set_arch_mach (obfd, bfd_get_arch (ibfd),
>> > + bfd_get_mach (ibfd)))
>> > + return FALSE;
>> > + }
>> > +
>> > + return TRUE;
>> > + }
>> > +
>> > + /* Update the output abiflags fp_abi using the computed fp_abi. */
>> > + obj_attribute *out_attr = elf_known_obj_attributes (obfd)[OBJ_ATTR_GNU];
>> > + mips_elf_tdata (obfd)->abiflags.fp_abi = out_attr[Tag_GNU_MIPS_ABI_FP].i;
>> > +
>> > +#define max(a,b) ((a) > (b) ? (a) : (b))
>> > + /* Merge abiflags. */
>> > + mips_elf_tdata (obfd)->abiflags.gpr_size
>> > + = max (mips_elf_tdata (obfd)->abiflags.gpr_size,
>> > + mips_elf_tdata (ibfd)->abiflags.gpr_size);
>> > + mips_elf_tdata (obfd)->abiflags.cpr1_size
>> > + = max (mips_elf_tdata (obfd)->abiflags.cpr1_size,
>> > + mips_elf_tdata (ibfd)->abiflags.cpr1_size);
>> > + mips_elf_tdata (obfd)->abiflags.cpr2_size
>> > + = max (mips_elf_tdata (obfd)->abiflags.cpr2_size,
>> > + mips_elf_tdata (ibfd)->abiflags.cpr2_size);
>> > +#undef max
>> > + mips_elf_tdata (obfd)->abiflags.ases
>> > + |= mips_elf_tdata (ibfd)->abiflags.ases;
>> > + mips_elf_tdata (obfd)->abiflags.isa_ext
>> > + |= mips_elf_tdata (ibfd)->abiflags.isa_ext;
>>
>> isa_ext should be handled by bfd_set_arch_mach.
>
> Are you saying we should ignore the isa_ext inputs and just use the new
> overall arch to set this?
Maybe bfd_set_arch_mach isn't the best place, but the reason I suggested it
was that the bfd_mach of the inputs should already reflect the architecture
in the input abiflags. That might mean adding some more bfd_machs if there
isn't one for every AFL_EXT_*.
The bfd_mach is also used to perform the compatibility check
(mips_mach_extends_p in the code quoted above). If the input bfd_mach isn't
correct at that point then we're not going to do the check properly.
> The potential problem with bfd_set_arch_mach is that it is called in a
> variety of different places and it can't set just the isa_ext field
> and then mark the abiflags as valid. I'm probably being dumb but I'm
> not sure how/where to hook this function either? I haven't quite got
> my head around BFD target vectors.
OK, I hadn't thought about the partial initialisation problem.
In that case I agree it might be better to keep it here. But setting
the output isa_ext (and isa_level and isa_rev?) should at least be done
when calling bfd_set_arch_mach in the code above. Please put the code
in a helper function though.
>> > +/* This is the struct we use to hold the module level set of options. Note
>> > + that we must set the isa field to ISA_UNKNOWN and the ASE fields to
>> > + -1 to indicate that they have not been initialized. */
>> >
>> > -/* 1 if -msingle-float, 0 if -mdouble-float. The default is 0. */
>> > -static int file_mips_single_float = 0;
>> > +static struct mips_set_options file_mips_opts =
>> > +{
>> > + /* isa */ ISA_UNKNOWN, /* ase */ 0, /* mips16 */ -1, /* micromips */ -1,
>> > + /* noreorder */ 0, /* at */ ATREG, /* warn_about_macros */ 0,
>> > + /* nomove */ 0, /* nobopt */ 0, /* noautoextend */ 0, /* insn32 */ FALSE,
>> > + /* gp32 */ -1, /* fp */ -1, /* arch */ CPU_UNKNOWN, /* sym32 */ FALSE,
>> > + /* soft_float */ FALSE, /* single_float */ FALSE,
>> > + /* mips_defer_fp_warn */ FALSE
>> > +};
>> >
>> > -/* True if -mnan=2008, false if -mnan=legacy. */
>> > -static bfd_boolean mips_flag_nan2008 = FALSE;
>> > +/* This is the struct we use to hold the current set of options. Note
>> > + that we must set the isa field to ISA_UNKNOWN and the ASE fields to
>> > + -1 to indicate that they have not been initialized. */
>> >
>> > static struct mips_set_options mips_opts =
>> > {
>> > /* isa */ ISA_UNKNOWN, /* ase */ 0, /* mips16 */ -1, /* micromips */ -1,
>> > /* noreorder */ 0, /* at */ ATREG, /* warn_about_macros */ 0,
>> > /* nomove */ 0, /* nobopt */ 0, /* noautoextend */ 0, /* insn32 */ FALSE,
>> > - /* gp32 */ 0, /* fp32 */ 0, /* arch */ CPU_UNKNOWN, /* sym32 */ FALSE,
>> > - /* soft_float */ FALSE, /* single_float */ FALSE
>> > + /* gp32 */ -1, /* fp */ -1, /* arch */ CPU_UNKNOWN, /* sym32 */ FALSE,
>> > + /* soft_float */ FALSE, /* single_float */ FALSE,
>> > + /* mips_defer_fp_warn */ FALSE
>> > };
>>
>> Let's not duplicate the comments like this -- please fuse them together
>> more naturally.
>
> OK. I expect you are only referring to large comments not the inline comments
> For field names.
Yeah, sorry, it was the duplication of the "Note ..." part.
>> > +/* Perform consistency checks on the current options. */
>> > +
>> > +static void
>> > +mips_check_options (struct mips_set_options *opts, bfd_boolean abi_checks)
>> > +{
>> > + /* Check the size of integer registers agrees with the ABI and ISA. */
>> > + if (opts->gp32 == 0 && !ISA_HAS_64BIT_REGS (opts->isa))
>> > + as_bad (_("`gp64' used with a 32-bit processor"));
>> > + else if (abi_checks == TRUE
>> > + && opts->gp32 == 1 && ABI_NEEDS_64BIT_REGS (mips_abi))
>> > + as_bad (_("`gp32' used with a 64-bit ABI"));
>> > + else if (abi_checks == TRUE
>> > + && opts->gp32 == 0 && ABI_NEEDS_32BIT_REGS (mips_abi))
>> > + as_bad (_("`gp64' used with a 32-bit ABI"));
>> > +
>> > + /* Check the size of the float registers agrees with the ABI and ISA. */
>> > + switch (opts->fp)
>> > + {
>> > + case 0:
>> > + if (!CPU_HAS_LDC1_SDC1 (opts->arch))
>> > + as_bad (_("`fpxx' used with a cpu lacking ldc1/sdc1 instructions"));
>> > + else if (opts->single_float == 1
>> > + || opts->soft_float == 1)
>> > + as_bad (_("`fpxx' cannot be used with `single-float' or `soft-float'"));
>> > + break;
>> > + case 64:
>> > + if (!ISA_HAS_64BIT_FPRS (opts->isa))
>> > + as_bad (_("`fp64' used with a 32-bit fpu"));
>> > + else if (abi_checks == TRUE
>> > + && ABI_NEEDS_32BIT_REGS (mips_abi)
>> > + && !ISA_HAS_MXHC1 (opts->isa))
>> > + as_warn (_("`fp64' used with a 32-bit ABI"));
>>
>> You said before that -mfp64 and -msingle-float were compatible, but I'm not
>> sure they are really. I think that should be an error too.
>
> This area is fuzzy at best. I've seen evidence both ways round in binutils
> (I can't point it out right now though) but GCC certainly only supports
> -mfp32 -msingle-float so I'd be happy with enforcing that in gas and
> dealing with any potential issues.
Thanks.
>> > + if ((regno & 1) != 0)
>> > + {
>> > + if (mips_opts.mips_defer_fp_warn == TRUE)
>> > + as_warn (_("Dangerous use of FP registers in fp%s when module is
> fp%s"),
>> > + (mips_opts.fp == 32) ? "32"
>> > + : (mips_opts.fp == 64) ? "64" : "xx",
>> > + (file_mips_opts.fp == 32) ? "32"
>> > + : (file_mips_opts.fp == 64) ? "64" : "xx");
>>
>> I don't think we should warn about code that's compatible with the
>> current ".set fp". How would you write a warning-free fp=64 ifunc?
>
> This is perhaps a non-intuitive piece of code but the mips_defer_fp_warn flag
> will only be set if the overall module was not FPXX, the current mode is
> not FPXX and the two do not match. The only way to support having both FP32
> and FP64 in the same module is if the overall module is FPXX. The
> warning handles
> the case of 'xx' when in reality it can never happen. It can only print
> fp32,fp64
> or fp64,fp32. The reason the overall module must be FPXX (or matching
> the current
> mode) is because that is the only way you get the choice of mode at
> runtime (FPXX)
> or the correct mode to start with (FP64).
So the warning is about cases like ".module fp=32 .... .set fp=64 .... "
and ".module fp=64 .... .set fp=32 .... "? I still think that if the user
has code in a .set fp=NN block, we should assume that they really do want
to write some code for NN-bit FPRs.
Why does the oddness of the register matter in these cases? I can see
why it would matter for xx because of the special call-save/call-clobber
rules, but why is it as special when the FPR sizes of the module and the
.set block are known? E.g. using an even FPR in fp=32 code is going to
clobber more registers than it would in fp=64 mode.
If we're going to treat this as a problem, I think we should do it when
the ".set" is first encountered.
>> > @@ -5322,13 +5527,12 @@ match_float_constant (struct mips_arg_info
> *arg, expressionS
>> *imm,
>> > /* Handle 64-bit constants for which an immediate value is best. */
>> > if (length == 8
>> > && !mips_disable_float_construction
>> > - /* Constants can only be constructed in GPRs and copied
>> > - to FPRs if the GPRs are at least as wide as the FPRs.
>> > - Force the constant into memory if we are using 64-bit FPRs
>> > - but the GPRs are only 32 bits wide. */
>> > - /* ??? No longer true with the addition of MTHC1, but this
>> > - is legacy code... */
>> > - && (using_gprs || !(HAVE_64BIT_FPRS && HAVE_32BIT_GPRS))
>> > + /* Constants can only be constructed in GPRs and copied to FPRs if
> the
>> > + GPRs are at least as wide as the FPRs or MTHC1 is available. */
>> > + && (using_gprs
>> > + || HAVE_64BIT_GPRS
>> > + || ISA_HAS_MXHC1 (mips_opts.isa)
>> > + || (HAVE_32BIT_FPRS && mips_opts.fp != 0))
>>
>> We should keep the bit about forcing it to memory otherwise.
>> Can you explain the HAVE_32BIT_FPRS && mips_opts.fp != 0 thing a bit more?
>> Maybe we should get rid of HAVE_32BIT_FPRS and HAVE_64BIT_FPRS now that
>> there are three alternatives and just test mips_opts.fp directly.
>> (Please do that as a separate patch though -- there are quite a few
>> different things going on in the current patch.)
>
> The final clause is to allow 64-bit constant generation when the value would
> need to be moved to FPRs using a pair of MTC1s (not MTHC1). I retained the
> HAVE_32BIT_FPRS clause as it expands to more than just fp != 64, but I don't
> understand why it is more complex than that. The fp != 0 could just as easily
> have been fp == 32 which is probably clearer and is there to ensure that
> we don't break the FPXX ABI by using an MTC1 for the high part of a 64-bit
> value.
>
> There are still only two widths of register. The fact we are targeting FPXX
> is only relevant in a small number of places, otherwise it is simply 32-bit
> registers. So I would be changing HAVE_32BIT_FPRS to fp != 64 anyway.
That's fine. I still think it's clearer, since it's fairly obvious that
fp != 64 means "not fp=64 mode", whereas it isn't as obvious that
HAVE_32BIT_FPRS means "fp=32 or fp=xx" mode. Having to combine checks
for HAVE_NNBIT_FPRS and mips_opts.fp seems a bit weird.
I'd missed that HAVE_32BIT_FPRS depends on the ISA. I'm not sure that's
a good idea either, but if we have to keep it, maybe a macro like FPR_SIZE
would be better than direct checks for mips_opts.fp.
>> > + if (prev_arch != file_mips_opts.arch
>> > + && ! bfd_set_arch_mach (stdoutput, bfd_arch_mips,
> file_mips_opts.arch))
>> > + as_warn (_("could not set architecture and machine"));
>>
>> I think we should do this unconditionally when checking the options
>> rather than here.
>
> OK. I was cautious as I didn't know if there would be any side effects
> of calling bfd_set_arch_mach unnecessarily.
Sorry, when I said "checking the options" I meant "checking the
file-level options". That should happen exactly once per assembly and
seems the right time to set the bfd_mach on the result.
>> > +@item 2 - Single-precision
>> > +This variant indicates that single-precision support is used. This is
>> > +generally taken to mean that the ABI is also modified such that
>> > +sizeof (double) == sizeof (float). This has an impact on calling
>> > +convention and callee-save behaviour.
>>
>> I'm not sure about the sizeof (double) thing: I think it's legitimate
>> to use -msingle-float with a 32-bit FPU and use software libraries
>> for double-precision.
>
> Granted. But I believe the intent of the existing single-precision ABI is
> that it is also -fshort-double in GCC terms (r5900 I believe). We would
> need to track both single-float and single-float short-double separately.
> There are no current plans (from us at least) to support just
> single-precision with softfloat doubles. The R6 spec re-introduces the
> idea of a single precision only FPU and this is planned to be primarily
> supported with short-doubles.
But emulated double-precision is definitely supported (and IIRC was also
used for r5900 GNU/Linux). Maybe:
Any double-precision operations must be emulated by software.
instead?
>> > +@node MIPS FP ABI Selection
>> > +@subsection Automatic selection of FP ABI
>> > +@cindex @code{.module fp=@var{nn}} directive, MIPS
>> > +In order to simplify and add safety to the process of selecting the
>> > +correct floating-point ABI, the assembler will automatically infer the
>> > +correct @code{.gnu_attribute 4, @var{n}} directive based on command line
>> > +options @code{.module} overrides and instruction usage. Where an explicit
>> > +@code{.gnu_attribute 4, @var{n}} directive has been seen then a warning
>> > +will be raised if it does not match an inferred setting.
>>
>> Obviously the "and instruction usage" is the contentious bit here :-)
>
> If we go with not having any safety net for hand written assembler then
> instruction usage does still need to be included in this decision in order
> to support getting modules that end up as FP 'ANY' i.e. no-float. Otherwise
> we must write-off the ability to generate modules without a specific FP ABI
> because there is no way to detect if a user has written .gnu_attribute 4,0
> nor is there a command-line/module option to assert it.
If no attributes or command-line options are given, we can use the default
command-line setting for the configuration, which I think at the moment
is always -mdouble-float. This will give a specific ABI.
This is just like the way that a file assembled without any other
information will get the default ISA level and CPU.
> Any thoughts on what to do with attribute 4,0? Since it is not very safe
> in GCC terms then we could just not support it I suppose.
We need a way for code that doesn't use FP to say that it doesn't care
about the FPU setting, so 4,0 seems fine to me.
> I expect you can guess that I'm still not convinced by providing no safety net
> at all for hand-written code :-) But I do understand the desire to keep things
> clear cut. The original plan would have avoided this issue as it defined new
> directives to get into fpxx. Perhaps it won't be an issue but I really wanted
> to be able to provide distribution maintainers with a tool to detect which
> objects remained FP32 even after the transition to an FPXX toolchain.
To be clear, I'm all in favour of warning about code that is wrong according
to the current mips_opts. My objections were about not trusting ".set"
and about having complicated attempts to infer the ABI.
If the code is modern enough to use .gnu_attribute, we'll warn or error
(maybe the latter is best) about it being incompatible with -mfpxx.
If the code isn't modern enough to use .gnu_attribute and doesn't use
.set fp=, we should warn about anything that is obviously incompatible
with fp=xx (although clearly the warning can't be perfect). In those
two cases we have the safety net.
If the code doesn't use .gnu_attribute but does use .set fp=, we simply
can't tell it apart from code is validly assuming a particular FPR size
(after appropriate checking).
Thanks,
Richard