<div><span style="font-family:arial,sans-serif;font-size:13px">Updated and XFAIL'ed unit tests for ADR:</span><br></div><div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><a href="https://github.com/garious/llvm/commit/882f3236b7cbdf91a988eaa34be451c38c7ff8f7">https://github.com/garious/llvm/commit/882f3236b7cbdf91a988eaa34be451c38c7ff8f7</a><br>
<div><br></div><div>Thanks,</div><div>Greg</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Dec 3, 2012 at 6:37 PM, Greg Fitzgerald <span dir="ltr"><<a href="mailto:garious@gmail.com" target="_blank">garious@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>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.</div>
<div><br></div><div>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.<br>
</div><div><br></div><div>For reference, here is my patch:</div><div><br></div><div><div><a href="https://github.com/garious/llvm/compare/master...patch;arm-adr-encoding" target="_blank">https://github.com/garious/llvm/compare/master...patch;arm-adr-encoding</a><br>
</div><div><br></div></div><div>Thanks,</div><div>Greg</div><div class="HOEnZb"><div class="h5"><div><br></div><div><br></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Sep 18, 2012 at 5:53 PM, Greg Fitzgerald <span dir="ltr"><<a href="mailto:garious@gmail.com" target="_blank">garious@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Owen,<br>
<div><br>
> Immediate displacements (as opposed to labels) are an _undocumented_<br>
> extension of ARM unified syntax<br>
<br>
</div>Thanks for the clarification. Diving deeper, I see the syntax is<br>
documented as the "alternative syntax" and is explicitly discouraged<br>
by ARM for this instruction.<br>
<br>
"""<br>
Note ARM recommends that where possible, software avoids using:<br>
• the alternative syntax for the ADR, LDC, LDC2, LDR, LDRB, LDRD,<br>
LDRH, LDRSB, LDRSH, PLD, PLI, PLDW, and VLDR<br>
instructions<br>
<div>"""<br>
<br>
<br>
> I'd much rather see the assembly parser enhanced to reject them altogether<br>
<br>
</div>I agree, we should make the parser reject this syntax.<br>
<div><br>
<br>
> the intended design of the MC layer is that it should not be possible to<br>
> construct an MCInst representing an unencodable instruction<br>
<br>
</div>I see, good rule. Is this, or other rules I should know about,<br>
documented somewhere?<br>
<br>
Thanks,<br>
Greg<br>
<div><div><br>
<br>
On Tue, Sep 18, 2012 at 2:53 PM, Owen Anderson <<a href="mailto:resistor@mac.com" target="_blank">resistor@mac.com</a>> wrote:<br>
> Greg,<br>
><br>
> 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.<br>
><br>
> 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.<br>
><br>
> --Owen<br>
><br>
><br>
> On Sep 17, 2012, at 10:27 AM, Greg Fitzgerald <<a href="mailto:gregf@codeaurora.org" target="_blank">gregf@codeaurora.org</a>> wrote:<br>
><br>
>>> There was a previous thread on this topic, but that bug<br>
>>> should have been closed as invalid, I believe.<br>
>><br>
>> Thanks Gordon. Stepan's patch would fix the problem with encoding<br>
>> immediates, but agree with Owen that it should break ADR when the argument<br>
>> is a label, because it would be shifted twice. The patch I submitted fixes<br>
>> only the immediates and doesn't affect fixups. It also makes sure the<br>
>> immediate is a multiple of 4 and within the range 0 to 1020. I've updated<br>
>> the unit tests and walked through relaxation in the debugger to verify the<br>
>> fix. I would have liked to see post-relaxation unit tests as well, but<br>
>> didn't spot any. If we have those for ADR or for other instructions, can<br>
>> someone kindly point me to an example?<br>
>><br>
>> Owen, Anton, can you please have a look at my patch?<br>
>><br>
>> Thanks,<br>
>> Greg<br>
>><br>
>><br>
>> -----Original Message-----<br>
>> From: Gordon Keiser [mailto:<a href="mailto:gkeiser@arxan.com" target="_blank">gkeiser@arxan.com</a>]<br>
>> Sent: Sunday, September 16, 2012 12:00 PM<br>
>> To: Greg Fitzgerald; <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
>> Subject: RE: [llvm-commits] [PATCH] Fixed ADR in ARM code generator<br>
>><br>
>><br>
>>> From: <a href="mailto:llvm-commits-bounces@cs.uiuc.edu" target="_blank">llvm-commits-bounces@cs.uiuc.edu</a><br>
>>> [mailto:<a href="mailto:llvm-commits-bounces@cs.uiuc.edu" target="_blank">llvm-commits-bounces@cs.uiuc.edu</a>] On Behalf Of Greg Fitzgerald<br>
>>> Sent: Friday, September 14, 2012 2:26 PM<br>
>>> To: <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
>>> Subject: [llvm-commits] [PATCH] Fixed ADR in ARM code generator<br>
>>><br>
>>> My first patch to LLVM, please review.<br>
>>><br>
>>> This addresses bug 13537: <a href="http://llvm.org/bugs/show_bug.cgi?id=13537" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=13537</a><br>
>>><br>
>>> Thanks,<br>
>>> Greg<br>
>>> --<br>
>>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,<br>
>>> hosted by The Linux Foundation<br>
>><br>
>> There was a previous thread on this topic, but that bug should have been<br>
>> closed as invalid, I believe.<br>
>> getThumbAdrLabelOpValue retrieves the branch target using the fixup type<br>
>> fixup_thumb_adr_pcrel_10, which is defined in ARMAsmBackend.cpp to return:<br>
>> ((Value - 4) >> 2) & 0xff<br>
>><br>
>> Original message on gmane<br>
>> <a href="http://permalink.gmane.org/gmane.comp.compilers.llvm.cvs/119492" target="_blank">http://permalink.gmane.org/gmane.comp.compilers.llvm.cvs/119492</a><br>
>> Someone with more extensive knowledge of the ARM backend can probably tell<br>
>> you more, you may be fixing something somewhat different.<br>
>><br>
>> Cheers,<br>
>> Gordon Keiser<br>
>> Software Development Engineer<br>
>> Arxan Technologies<br>
>> <a href="mailto:gkeiser@arxan.com" target="_blank">gkeiser@arxan.com</a> <a href="http://www.arxan.com" target="_blank">www.arxan.com</a><br>
>><br>
>><br>
>><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>