[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 inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMConstantIslandPass.cpp:1372
+    // blocks it creates is not in the middle of an IT block.
+    while (MI->getOpcode() != ARM::t2IT &&
+           getITInstrPredicate(*MI, PredReg) != ARMCC::AL) {
----------------
dmgreen wrote:
> I'm not sure I understand this (existing) code completely. Would it be possible for this new loop to walk _past_ the end of BaseInsertOffset, making a CPE out of range of the user? Because each iteration of this outer loop can now increase Offset by more than just 2/4 (getInstSizeInBytes), and the --MI at the end of the loop might not get us back in-range.
> 
> Or would it be possible to miss instructions that should be handled in the "if (CPUIndex < NumCPUsers && CPUsers[CPUIndex].MI == &*MI)" block above because they are inside IT blocks?
An IT block is at most 4 instructions plus the "it" itself (18 bytes).  The code normally tries to choose the last possible point to split, and all the relevant instructions support much larger offsets, so we should never choose a split point inside the same it block as the constant pool reference.

But we're hitting the "This could point off the end of the block" case here; for reasons I don't really understand, this cuts off the iteration well before the end of the block.  Probably the right fix is to change the computation of BaseInsertOffset so it doesn't cut off the loop before the first legal split point.


================
Comment at: llvm/lib/Target/ARM/ARMConstantIslandPass.cpp:1400
   // We really must not split an IT block.
   LLVM_DEBUG(unsigned PredReg; assert(
                  !isThumb || getITInstrPredicate(*MI, PredReg) == ARMCC::AL));
----------------
dmgreen wrote:
> This assert shouldn't be in a LLVM_DEBUG. Can you move it out whilst you are here?
I think it's written this way to avoid an unused variable warning for PredReg.  But yes, it should be using `#ifndef NDEBUG`, not LLVM_DEBUG.


================
Comment at: llvm/test/CodeGen/ARM/constant-islands-split-IT.mir:93
+    renamable $d0 = VLDRD %const.1, 0, 0, $cpsr, implicit $itstate :: (load 8 from constant-pool)
+    renamable $d1 = VLDRD %const.2, 0, 0, $cpsr, implicit $itstate :: (load 8 from constant-pool)
+    renamable $d2 = VLDRD %const.0, 0, 0, $cpsr, implicit $itstate :: (load 8 from constant-pool)
----------------
Do you know why we're trying to split this block in the first place?  It should be possible to place all the necessary constant pool entries after the call to __stack_chk_fail.


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

https://reviews.llvm.org/D64621