This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
[ping][PATCH] MIPS: Document the use of FP_D in MIPS16 mode
- From: "Maciej W. Rozycki" <macro at codesourcery dot com>
- To: Richard Sandiford <rdsandiford at googlemail dot com>
- Cc: binutils at sourceware dot org
- Date: Wed, 3 Aug 2011 15:55:21 +0100 (BST)
- Subject: [ping][PATCH] MIPS: Document the use of FP_D in MIPS16 mode
- References: <alpine.DEB.1.10.1102221625230.20460@tp.orcam.me.uk> <87boxg5nyo.fsf@firetop.home> <alpine.DEB.1.10.1107020155260.4083@tp.orcam.me.uk> <87boxdcf2b.fsf@firetop.home> <alpine.DEB.1.10.1107041953320.4083@tp.orcam.me.uk> <87ei254wmb.fsf@firetop.home> <alpine.DEB.1.10.1107042309460.4083@tp.orcam.me.uk>
Hi Richard,
Where are we with the change below?
Maciej
On Mon, 4 Jul 2011, Maciej W. Rozycki wrote:
> On Mon, 4 Jul 2011, Richard Sandiford wrote:
>
> > > BTW, I've been wondering about this construct:
> > >
> > > return mask & ~0;
> > >
> > > you've introduced in a couple of places.
> >
> > Oops. Fixed below.
>
> Hmm, makes sense indeed, thanks. Except that it didn't pop up in testing
> which is worrisome and means that all this branch swapping stuff is not
> really terribly well covered. I think we should cook up something more
> systematic than the couple of scattered cases we already have.
>
> > > Also why "can not" instead of "cannot"? My understanding of the
> > > subtleties of English is this essentially means we "are allowed not to
> > > swap" rather than we "are not allowed to swap". The usual disclaimer
> > > applies of course. ;)
> >
> > Those comments were already there. I just moved them around.
>
> Fair enough. I got a notion of being otherwise, but now that I have
> double-checked you are right. Sorry about the confusion.
>
> In that case though I'm tempted to adjust the comments I'll be tweaking
> anyway with the microMIPS changes, if you don't mind that is.
>
> > > Finally I think the change below is needed as well. We shouldn't be
> > > relying on the meaning of FP_D for MIPS16 code as opcode flags are assumed
> > > to have a different meaning in the MIPS16 table. Granted, the location of
> > > the FP_D bit hasn't been assigned to anything (yet), so it's guaranteed to
> > > be zero and the check is harmless,
> >
> > Yes. I checked that before writing it this way. It's guaranteed to be
> > harmless anyway, because "until" MIPS16 gets hardware FP support, we'd
> > never have any set bits.
>
> Actually, I was more worried about this bit being reused for something
> else as a distinct MIPS16_* flag and then hitting this condition
> inadvertently. There is a place in append_insn() using the FPR mask
> unconditionally even in the MIPS16 mode (quite rightfully IMO). Granted,
> it's only used for the calculation of .fmask, that is less of importance
> now that we have DWARF-2, but still.
>
> How about making a suitable comment in include/opcode/mips.h, preferably
> following all the other MIPS16_* flags, that the bit position used for the
> FP_D flag is already reserved for MIPS16 code too? There's a terse
> description already there covering the shared flags. I think a change
> like below should be enough.
>
> I think INSN_ISA3 should be moved to the beginning and made clearly
> distinct. It took me a while to notice it's there. I have decided these
> changes are too trivial to be kept separate, so I've made them both at
> once.
>
> > I structured the code to be consistent with the GPR case. If you feel
> > strongly about it though, feel free to move the conditions inside the
> > existing "!mips_opts.mips16" check.
>
> I just feel no confidence that the notion of such an assumption will
> stand for the next half a dozen years or so. The chance for the MIPS16
> ASE being extended any further is probably slim, but the lifespan of
> binutils is long enough to be careful about any assumptions and about
> long-term maintenance of any piece of code. We seem to keep discovering
> obscure oddities all the time despite quite a strict maintenance regime.
>
> As long as include/opcode/mips.h is updated accordingly I don't insist on
> my previous change though.
>
> Maciej
>
> 2011-07-04 Maciej W. Rozycki <macro@codesourcery.com>
>
> include/opcode/
> * mips.h: Document the use of FP_D in MIPS16 mode. Adjust the
> order of flags documented.
>
> Index: include/opcode/mips.h
> ===================================================================
> RCS file: /cvs/src/src/include/opcode/mips.h,v
> retrieving revision 1.73
> diff -u -p -r1.73 mips.h
> --- include/opcode/mips.h 28 Feb 2011 16:06:51 -0000 1.73
> +++ include/opcode/mips.h 4 Jul 2011 22:57:01 -0000
> @@ -1132,6 +1132,9 @@ extern int bfd_mips_num_opcodes;
>
> /* The following flags have the same value for the mips16 opcode
> table:
> +
> + INSN_ISA3
> +
> INSN_UNCOND_BRANCH_DELAY
> INSN_COND_BRANCH_DELAY
> INSN_COND_BRANCH_LIKELY (never used)
> @@ -1140,7 +1143,7 @@ extern int bfd_mips_num_opcodes;
> INSN_WRITE_HI
> INSN_WRITE_LO
> INSN_TRAP
> - INSN_ISA3
> + FP_D (never used)
> */
>
> extern const struct mips_opcode mips16_opcodes[];
>