[PATCH] Fix assembling of Thumb2 branch instructions.

[Mihail Popa]

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/20130807/4fc8f31f/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: LLVM-820.t2b.patch
Type: application/octet-stream
Size: 19966 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130807/4fc8f31f/attachment.obj>