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

Tue Dec 4 18:47:42 PST 2012

Updated and XFAIL'ed unit tests for ADR:

https://github.com/garious/llvm/commit/882f3236b7cbdf91a988eaa34be451c38c7ff8f7

Thanks,
Greg

On Mon, Dec 3, 2012 at 6:37 PM, Greg Fitzgerald <garious at gmail.com> wrote:

> 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/20121204/b46a99d7/attachment.html>