[PATCH] Fix implementation for Thumb ADR instruction

[Mihail Popa]

New patch attached.



On Tue, Jul 2, 2013 at 4:12 PM, Mihail Popa <mihail.popa at gmail.com> wrote:

> Tim,
>
> even if it only applies to the branches, there will be plenty of
> occurrences:
>
> 1. branch conditional T1 encoding - signed, 8 bit, shift 1
> 2. branch unconditional T2 encoding - signed, 11 bit, shift 1
> 3. branch conditional T3 encoding - signed 24 bit, shift 1
> 4. branch linked conditional T2 encoding - signed 24 bit, shift 2
>
> For load and store literal, with sign-magnitude format:
>
> 1. T1 encoding: unsigned, 8 bit, shift 2
> 2. T2 encoding: signed 12 bit, no shift
>
> These cover only functional issues, where an encoding that is too short is
> chosen
> over the one of correct length.
>
> There are probably more examples if we add constraints for the purpose of
> diagnostics
> and error checking.
>
> I do agree it'd be probably better to split the template in unsigned, 2's
> complement signed
> and magnitude signed.
>
> Mihai
>
>
>
> On Tue, Jul 2, 2013 at 12:56 PM, Tim Northover <t.p.northover at gmail.com>wrote:
>
>> Hi Mihail,
>>
>> Thanks for working on this, it's looking better now.
>>
>> > Please see attached the revised patch. I have made the template
>> function and
>> > the new operand classes clearly refer to PC-relative addresses
>>
>> So how widely are you planning to apply this method? I don't think it
>> will account for the other forms of ADR for example, which have the
>> same quirk as LDR that Min = -Max. It seems it could work for:
>>   + Branches
>>   + T1 encoding of ADR
>>   + ADD, SUB (even if not PC-rel, assuming we're not going to support
>> "ADD r0, r0, #-4" as an alias).
>>   + MOVW/MOVT
>>
>> And it would fail for:
>>   + Other encodings of ADR (as LLVM implements them, with +/- in same
>> MCInst)
>>   + Loads & stores.
>>
>> I don't think the "PCOffset" is a particularly better description than
>> "MemoryOffset" for those instructions, given that of the three kinds
>> of PC-offsetting instructions (ADR, LDR (lit), B) it works for 1.5 of
>> them.
>>
>> Perhaps it's too general, and should split into an unsigned (added
>> now) and two signed variants (added as and when they're needed), not
>> controlled by templates?
>>
>> > and changed the shifting policy.
>>
>> Thanks.
>>
>> Some more boundary tests would probably be good (e.g. that "ADR r0,
>> #1020" and "ADR r0, #-4" really are the edges of what's representable
>> by the T1 encoding)?
>>
>> Regards.
>>
>> Tim.
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130702/8b216949/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: LLVM-788.tadr.patch
Type: application/octet-stream
Size: 8435 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130702/8b216949/attachment.obj>