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: Mon, 04 Jul 2011 20:30:00 -0000 [thread overview]
Message-ID: <alpine.DEB.1.10.1107041953320.4083@tp.orcam.me.uk> (raw)
In-Reply-To: <87boxdcf2b.fsf@firetop.home>
On Sat, 2 Jul 2011, Richard Sandiford wrote:
> > Hmm, does it? I did verify it both on a GNU/Linux and an ELF target. It
> > still passes for me, except that:
> >
> > + Advance PC by 24 to 0x64
> >
> > is required instead as with my intended-to-be-sent-originally change.
> > Likewise:
> >
> > + Advance PC by 23 to 0x40
> >
> > for the MIPS16 dump. Which target is failing for you? Perhaps the cause
> > was merely the outdated version of the change, sigh...
>
> Ah, could be. It was mips64-elf. Maybe I'd got confused and the
> original didn't pass for GNU/Linux after all. Sorry about that.
I have checked mips64-elf to be sure and this test case passes in its
updated form (there are some unrelated failures for this target that
shouldn't happen though).
> > I have regenerated and revalidated the change now and I'm ready to push
> > it, but I'd prefer the final DWARF-2 instructions to be matched exactly.
> > I made the effort to make the source alignment-agnostic and I'd prefer to
> > keep the final offsets to be verified literally so that any unexpected
> > discrepancies are caught.
>
> Agreed.
I'm glad to hear this for once. ;)
> > 2011-07-02 Maciej W. Rozycki <macro@codesourcery.com>
> >
> > gas/
> > * config/tc-mips.c (append_insn): Make sure DWARF-2 location
> > information is properly adjusted for branches that get swapped.
> >
> > gas/testsuite/
> > * gas/mips/loc-swap.d: New test case for DWARF-2 location with
> > branch swapping.
> > * gas/mips/loc-swap-dis.d: Likewise.
> > * gas/mips/mips16\@loc-swap.d: Likewise, MIPS16 version.
> > * gas/mips/mips16\@loc-swap-dis.d: Likewise.
> > * gas/mips/loc-swap.s: Source for the new tests.
> > * gas/mips/mips.exp: Run the new tests.
>
> OK.
Applied now, thanks.
BTW, I've been wondering about this construct:
return mask & ~0;
you've introduced in a couple of places. The masking operation seems a
no-op to me -- given that both mask and the function's return type are
defined as unsigned int. Am I missing anything?
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. ;)
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, but I think we better avoided this kind
of overloading. Do you agree?
Alternatively, given that there's no FP support in the MIPS16 mode, the
two functions could simply return straight away in that case and then let
the rest of the bodies to stay outside a conditional block.
2011-07-04 Maciej W. Rozycki <macro@codesourcery.com>
gas/
* config/tc-mips.c (fpr_read_mask, fpr_write_mask): Avoid the
FP_D check in MIPS16 mode.
Maciej
binutils-gas-mips-fp-d.diff
Index: binutils-fsf-trunk-quilt/gas/config/tc-mips.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/config/tc-mips.c 2011-07-02 19:30:04.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/config/tc-mips.c 2011-07-02 19:32:22.000000000 +0100
@@ -2519,7 +2519,7 @@ fpr_read_mask (const struct mips_cl_insn
}
/* Conservatively treat all operands to an FP_D instruction are doubles.
(This is overly pessimistic for things like cvt.d.s.) */
- if (HAVE_32BIT_FPRS && (pinfo & FP_D))
+ if (!mips_opts.mips16 && HAVE_32BIT_FPRS && (pinfo & FP_D))
mask |= mask << 1;
return mask;
}
@@ -2548,7 +2548,7 @@ fpr_write_mask (const struct mips_cl_ins
}
/* Conservatively treat all operands to an FP_D instruction are doubles.
(This is overly pessimistic for things like cvt.s.d.) */
- if (HAVE_32BIT_FPRS && (pinfo & FP_D))
+ if (!mips_opts.mips16 && HAVE_32BIT_FPRS && (pinfo & FP_D))
mask |= mask << 1;
return mask;
}
next prev parent reply other threads:[~2011-07-04 19:35 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 [this message]
2011-07-04 22:20 ` Richard Sandiford
2011-07-05 0:28 ` Maciej W. Rozycki
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.1107041953320.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).