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

[Simi Pallipurath via Phabricator via llvm-commits]

simpal01 marked an inline comment as done.
simpal01 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) {
----------------
efriedma wrote:
> 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.
Yes. It is possible for this new loop to walk past the end of BaseInsertOffset and can make a CPE out of range of the user. But i thought since this pass iteratively place or move around the constant pools untill all the CPE is in range with the corresponding CPE user, this out of range can catch and make in range in the next iteration. Not sure if this is an optimal solution though!!


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

https://reviews.llvm.org/D64621