From: "Maciej W. Rozycki" <macro@codesourcery.com>
To: Richard Sandiford <rdsandiford@googlemail.com>
Cc: binutils@sourceware.org
Subject: Re: [PATCH] MIPS/GAS: Fix DWARF-2 with branch swapping for MIPS16 code
Date: Tue, 05 Jul 2011 00:28:00 -0000 [thread overview]
Message-ID: <alpine.DEB.1.10.1107042309460.4083@tp.orcam.me.uk> (raw)
In-Reply-To: <87ei254wmb.fsf@firetop.home>
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[];
next prev parent reply other threads:[~2011-07-04 22:58 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-02-23 13:39 Maciej W. Rozycki
2011-06-29 21:26 ` Richard Sandiford
2011-07-02 1:20 ` Maciej W. Rozycki
2011-07-02 7:33 ` Richard Sandiford
2011-07-04 20:30 ` Maciej W. Rozycki
2011-07-04 22:20 ` Richard Sandiford
2011-07-05 0:28 ` Maciej W. Rozycki [this message]
2011-07-05 11:29 ` Richard Sandiford
2011-08-03 14:55 ` [ping][PATCH] MIPS: Document the use of FP_D in MIPS16 mode Maciej W. Rozycki
2011-08-03 19:47 ` Richard Sandiford
2011-08-03 21:13 ` Maciej W. Rozycki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.DEB.1.10.1107042309460.4083@tp.orcam.me.uk \
--to=macro@codesourcery.com \
--cc=binutils@sourceware.org \
--cc=rdsandiford@googlemail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).