[PATCH] D64621: [ARM] Make sure that the constant pool does not keep in the middle of an IT block.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

This makes sense to me... but I'm not sure this handles all possible cases.  In particular, consider the case where both an instruction that needs a constant pool and the block's terminator are inside the it block.  Then the split point is after a terminator, so the code still doesn't work.  This might be hard to trigger, though... we don't normally put terminators inside it blocks.  A predicated unconditional branch generally turns into a conditional branch, not an unconditional branch in an it block.  And we don't support predicating jump table branches.  Actually, I think the only branch we predicate using an it block is tBRIND (which is generated from an IR indirectbr).  But it's still possible to hit that case, and we could add more similar cases in the future.  (A quick example where an it block contains both a terminator and non-terminator instruction: `echo "int f(void *g(void*,void*), void **q) { void*p = g(&&X, &&Y); if (p){*q=p; goto *p;} return 7; X: return 5; Y: return 6; }" | clang -x c - -o - -S -mllvm -stop-after=arm-cp-islands --target=armv7-eabi -O2 -mthumb`.)

So to make sure this works in general, I see two options:

1. Split the it block by inserting a new it instruction.
2. Fix the algorithm so it doesn't force splitting a block when it clearly doesn't need to be split.  I haven't dug into the algorithm here really carefully, but it doesn't make sense to split a block when the constant pool reference is close to the end of the block.  (I'm not sure exactly how to define "close", but it probably includes constant pool references within 50 bytes of the end of the block, given all pc-relative loads can reach data over 500 bytes away.)

We could merge the current patch anyway, though; even if it doesn't fix everything, it shouldn't break any constructs that aren't already broken.



================
Comment at: llvm/lib/Target/ARM/ARMConstantIslandPass.cpp:1328
                     << " up=" << UPad << '\n');
-
+  MachineBasicBlock::iterator MI = UserMI;
   // This could point off the end of the block if we've already got constant
----------------
I think moving the variable here is making the code less clear; better to just use a separate variable inside the if statement.


================
Comment at: llvm/lib/Target/ARM/ARMConstantIslandPass.cpp:1359
+          (BaseInsertOffset <= (Offset + TII->getInstSizeInBytes(*MI))))
+        BaseInsertOffset = Offset + TII->getInstSizeInBytes(*MI) + 1;
+    }
----------------
It's probably more readable to refactor the logic to compute the "next" possible offset, then perform the std::max() afterwards.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64621/new/

https://reviews.llvm.org/D64621