[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