[llvm-commits] [PATCH] Fixed ADR in ARM code generator

Mon Dec 3 18:37:18 PST 2012

Revisiting this one from back in September...  I hadn't been too concerned
about it since no IR maps to this syntax, but as we've been talking about
using llvm-mc as a general purpose assembler, it seems like we should
address that the ARM backend generates the wrong encoding for this syntax.
 At the very least, I hope we can update the unit-tests and XFAIL them.

Last time we discussed this patch (and a similar patch for Branch
instructions with an immediate), we got hung up on how to encode the
immediate in the MC layer.  I wonder if folks may have changed their
opinion here, or might be willing to offer up a competing patch that
pre-encodes (bit-shifts) MC operands at each constructor.

For reference, here is my patch:

https://github.com/garious/llvm/compare/master...patch;arm-adr-encoding

Thanks,
Greg

On Tue, Sep 18, 2012 at 5:53 PM, Greg Fitzgerald <garious at gmail.com> wrote:

> Owen,
>
> > Immediate displacements (as opposed to labels) are an _undocumented_
> > extension of ARM unified syntax
>
> Thanks for the clarification.  Diving deeper, I see the syntax is
> documented as the "alternative syntax" and is explicitly discouraged
> by ARM for this instruction.
>
> """
> Note ARM recommends that where possible, software avoids using:
> • the alternative syntax for the ADR, LDC, LDC2, LDR, LDRB, LDRD,
> LDRH, LDRSB, LDRSH, PLD, PLI, PLDW, and VLDR
> instructions
> """
>
>
> > I'd much rather see the assembly parser enhanced to reject them
> altogether
>
> I agree, we should make the parser reject this syntax.
>
>
> > the intended design of the MC layer is that it should not be possible to
> > construct an MCInst representing an unencodable instruction
>
> I see, good rule.  Is this, or other rules I should know about,
> documented somewhere?
>
> Thanks,
> Greg
>
>
> On Tue, Sep 18, 2012 at 2:53 PM, Owen Anderson <resistor at mac.com> wrote:
> > Greg,
> >
> > As I explained in another thread, the intended design of the MC layer is
> that it should not be possible to construct an MCInst representing an
> unencodable instruction.  Because of that, the design is such that
> immediate values of branches offsets much be pre-shifted as appropriate
> when constructing the MCInst itself.  Otherwise, it would be possible to
> construct a branch with an odd displacement value, which cannot be encoded.
> >
> > I don't consider the current behavior to be incorrect.  Immediate
> displacements (as opposed to labels) are an _undocumented_ extension of ARM
> unified syntax, and as such their interpretation reflects the assembler's
> internal model.  I'd much rather see the assembly parser enhanced to reject
> them altogether than rework the MC model to make them "work" in some
> abstract sense.
> >
> > --Owen
> >
> >
> > On Sep 17, 2012, at 10:27 AM, Greg Fitzgerald <gregf at codeaurora.org>
> wrote:
> >
> >>> There was a previous thread on this topic, but that bug
> >>> should have been closed as invalid, I believe.
> >>
> >> Thanks Gordon.  Stepan's patch would fix the problem with encoding
> >> immediates, but agree with Owen that it should break ADR when the
> argument
> >> is a label, because it would be shifted twice.  The patch I submitted
> fixes
> >> only the immediates and doesn't affect fixups.  It also makes sure the
> >> immediate is a multiple of 4 and within the range 0 to 1020.  I've
> updated
> >> the unit tests and walked through relaxation in the debugger to verify
> the
> >> fix.  I would have liked to see post-relaxation unit tests as well, but
> >> didn't spot any.  If we have those for ADR or for other instructions,
> can
> >> someone kindly point me to an example?
> >>
> >> Owen, Anton, can you please have a look at my patch?
> >>
> >> Thanks,
> >> Greg
> >>
> >>
> >> -----Original Message-----
> >> From: Gordon Keiser [mailto:gkeiser at arxan.com]
> >> Sent: Sunday, September 16, 2012 12:00 PM
> >> To: Greg Fitzgerald; llvm-commits at cs.uiuc.edu
> >> Subject: RE: [llvm-commits] [PATCH] Fixed ADR in ARM code generator
> >>
> >>
> >>> From: llvm-commits-bounces at cs.uiuc.edu
> >>> [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Greg Fitzgerald
> >>> Sent: Friday, September 14, 2012 2:26 PM
> >>> To: llvm-commits at cs.uiuc.edu
> >>> Subject: [llvm-commits] [PATCH] Fixed ADR in ARM code generator
> >>>
> >>> My first patch to LLVM, please review.
> >>>
> >>> This addresses bug 13537: http://llvm.org/bugs/show_bug.cgi?id=13537
> >>>
> >>> Thanks,
> >>> Greg
> >>> --
> >>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> >>> hosted by The Linux Foundation
> >>
> >> There was a previous thread on this topic, but that bug should have been
> >> closed as invalid, I believe.
> >> getThumbAdrLabelOpValue retrieves the branch target using the fixup type
> >> fixup_thumb_adr_pcrel_10, which is defined in ARMAsmBackend.cpp to
> return:
> >> ((Value - 4) >> 2) & 0xff
> >>
> >> Original message on gmane
> >> http://permalink.gmane.org/gmane.comp.compilers.llvm.cvs/119492
> >> Someone with more extensive knowledge of the ARM backend can probably
> tell
> >> you more, you may be fixing something somewhat different.
> >>
> >> Cheers,
> >> Gordon Keiser
> >> Software Development Engineer
> >> Arxan Technologies
> >> gkeiser at arxan.com  www.arxan.com
> >>
> >>
> >>
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121203/c548479e/attachment.html>