[PATCH] Fix assembling of Thumb2 branch instructions.

Mihail Popa mihail.popa at gmail.com
Fri Aug 9 02:08:31 PDT 2013


respectful ping


On Wed, Aug 7, 2013 at 1:09 PM, Mihail Popa <mihail.popa at gmail.com> wrote:

> Hi Tim.
>
> Please look at the attached changes.
> I created a custor AsmMatchConverter and simplified the logic a bit.
> Also added extra boundary condition tests. (and fixed style)
>
> Regards,
> Mihai
>
>
> On Tue, Aug 6, 2013 at 4:45 PM, Tim Northover <t.p.northover at gmail.com>wrote:
>
>> Hi Mihai,
>>
>> This really is a nasty one, isn't it? I don't like the fact that
>> validateInstruction is now modifying its argument; seems like a bad
>> precedent to set (all of the others just report on what's present as
>> far as I can see). How about moving the mutation logic into an
>> AsmMatchConverter method applied to these four instructions? You'd
>> still need a stub in validateInstruction to actually check immediates,
>> but it could be much simpler and really just do validation.
>>
>> Other than that, the biggest thing I saw was a possible error:
>>
>> +    if(op->isImm() && !op->isSignedOffset<11, 1>())
>> +    {
>> +      if(!isThumbTwo() || !op->isSignedOffset<25, 1>())
>>
>> Shouldn't that be isSignedOffset<24, 1>? Which suggests some error
>> tests would be a good idea (well, the would even if this is correct).
>> A few just-out-of-range immediates to make sure the messages are doing
>> what they should be.
>>
>> For the first block of classification logic, might a more
>> instruction-centric block be clearer?
>>
>> IsAL = ...;
>> switch(Inst.getOpcode()) {
>> case ARM::t2Bcc:
>>   if (inITBlock() || IsAL) Inst.setOpcode(ARM::t2B);
>>   break;
>> case ARM::t2B:
>>   if (!inITBlock() && !IsAL) Inst.setOpcode(ARM::t2Bcc);
>>   break;
>> [...same for tB and tBcc...]
>> }
>>
>> I'll leave it to your discretion, but that seems to make it more
>> obvious that we only care about these 4 instructions, and that the two
>> conditions are disjoint & complementary.
>>
>> Regardless of what you pick there, a minor nit is that the braces are
>> not in LLVM style.
>>
>> Cheers.
>>
>> Tim.
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130809/75e8b735/attachment.html>


More information about the llvm-commits mailing list