[PATCH] Fix assembling of Thumb2 branch instructions.
Tim Northover
t.p.northover at gmail.com
Tue Aug 6 08:45:42 PDT 2013
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.
More information about the llvm-commits
mailing list