[llvm-commits] [PATCH] thumb t2b fix

[Greg Fitzgerald]

My last patch wasn't accepted, I believe, because it didn't pre-encode the
immediate in the MC instruction.  Unlike my ADR patch, in this patch, it
was more obvious that pre-encoding isn't a good choice because of the extra
bit-twiddling.  That previous patch can no longer be merged cleanly because
of changes, but I notice that some encodings are still broken.  Here's a
patch that only updates and XFAIL's the unit-tests.  If you are interested
in a patch that doesn't pre-encode the immediate, I'd be happy to update my
previous patch to work with the latest code.  Otherwise, I hope you might
accept that small patch that helps track the existing issue.

https://github.com/garious/llvm/commit/916c4badd816178da9fdbac5b5ed2331a7201f98

Thanks,
Greg




On Thu, Sep 27, 2012 at 10:00 AM, Greg Fitzgerald <garious at gmail.com> wrote:

> Ping.
>
> same patch attached
>
> On Wed, Sep 26, 2012 at 10:27 AM, Greg Fitzgerald <garious at gmail.com>
> wrote:
> > The attached patch is the same as before but includes a missing
> > function declaration.
> >
> > Thanks,
> > Greg
> >
> >
> > On Mon, Sep 24, 2012 at 5:15 PM, Greg Fitzgerald <garious at gmail.com>
> wrote:
> >>> I believe that "target" only really exists in the .td file and cannot
> >>> be anything but S:J1:J2:imm10:imm11 by definition (it's assigned
> >>> directly to Inst{}!).
> >>
> >> 'target' maps to the input value to the decoder method and the return
> >> value of the encoder method.
> >>
> >>
> >>> I think Owen would prefer that getImm() returns S:J1:J2:imm10:imm11
> too.
> >>> He specifically refers to the MCOperand as having that value.
> >>
> >> Since no instructions in the ARM backend are currently written this
> >> way, when making bug fixes, can we aim to be consistent with the
> >> existing code?
> >>
> >> The more I think about it, I'm really not on board with the long-term
> >> plan of exposing the encoding details to the caller of getImm().  I
> >> think that method should return 'imm32', not 'S:J1:J2:imm10:imm11'.
> >> Returning imm32 keeps relaxation and instruction selection simple.
> >> But in any case, can we please move this aspect of the discussion to
> >> another thread?  If we're going to go this route, we should make the
> >> change to all ARM instructions at once, and as a separate patch.
> >>
> >> Attached is an updated patch ready for review.  It corrects an
> >> encoding bug in the J1/J2 bits, adds more unit tests, and simplifies
> >> the code by making a symmetric encoder/decoder for 'target'.
> >>
> >> Thanks,
> >> Greg
> >>
> >>
> >>
> >> On Thu, Sep 20, 2012 at 1:20 PM, Tim Northover <t.p.northover at gmail.com>
> wrote:
> >>> Hi Greg,
> >>>
> >>>> Thanks for the review Tim.  Just to verify, when you say "MCInst's
> >>>> immediate", we should distinguish the value returned by "getImm()"
> >>>> from the value in 'target' in the .td file.  I believe getImm() should
> >>>> return the sign-extended 'imm32' and that 'target' can contain
> >>>> 'S:J1:J2:imm10:imm11'.
> >>>
> >>> I believe that "target" only really exists in the .td file and cannot
> >>> be anything but S:J1:J2:imm10:imm11 by definition (it's assigned
> >>> directly to Inst{}!). (Intepreting the words of the prophet ;-)) I
> >>> think Owen would prefer that getImm() returns S:J1:J2:imm10:imm11 too.
> >>> He specifically refers to the MCOperand as having that value.
> >>>
> >>>> Otherwise, you're asking the user to do
> >>>> redundant bit-twiddling during instruction selection and relaxation,
> >>>
> >>> At a casual glance, selection is likely fine (it's just going to give
> >>> a label with an attached fixup: getImm() == 0 is acceptable).
> >>> Relaxation has to account for all the complexity regardless of which
> >>> route is taken.
> >>>
> >>> The other relevant bits are:
> >>> + Disassembler: complex now; no special handling in the proposed
> solution.
> >>> + InstPrinter: simple now; more complex in proposed solution.
> >>> + Encoder: complex now; simple in proposed solution.
> >>> + AsmParser: simple now; complex in proposed solution.
> >>>
> >>> We're shuffling the complexity around to achieve two goals:
> >>> 1. Fits in with other instructions in having operands to an MCInst
> >>> already encoded where possible.
> >>> 2. Make and MCInst is intrinsically valid (not entirely convinced on
> >>> this one, but it's certainly a step closer to that goal than the
> >>> status quo).
> >>>
> >>>> My interpretation of Owen's suggestion is to have a symmetric
> >>>> encoder/decoder on 'target' and remove the decoder on the 't2B'
> >>>> definition.
> >>>
> >>> I'm really not sure what you mean by "target"here. The only reference
> >>> I see is an arbitrary field name in the .td file. During runtime the
> >>> primary place this "target" exists is as an MCOperand. Various
> >>> components have to either create or interpret that value.
> >>>
> >>>> My preference, generally speaking, is to go "minimally invasive" when
> fixing a
> >>>> bug, and then refactor as a separate "cleanup" patch.[...] Sound good?
> >>>
> >>> I obviously have no problem with the first part of that, but I think
> >>> you're misinterpreting Owen's goals for the long term.
> >>>
> >>> Tim.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121204/c5d0d02b/attachment.html>