[PATCH] D29331: [LICM] Hoist loads that are dominated by invariant.start intrinsic, and are invariant in the loop.

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy accepted this revision.
sanjoy added a comment.
This revision is now accepted and ready to land.

lgtm with nits



================
Comment at: lib/Transforms/Scalar/LICM.cpp:483
 
+// This function checks if the load is dominated by an invariant.start at the
+// same location, and return true for invariance. If we find an invariant.end,
----------------
 - s/the load/\p LI/ for clarification
 - Mention that we also check the size
 - "If we find an invariant end" - slightly misleading, since we never actually look for one.  I'd rather phrase the comment as "Try to prove that \p LI is loading from a location invariant within the loop \p CurLoop by looking for an invariant_start intrinsic.  Return true ..."


================
Comment at: lib/Transforms/Scalar/LICM.cpp:489
+                                  Loop *CurLoop) {
+  Value *LoadOp = LI->getOperand(0);
+  const DataLayout &DL = LI->getModule()->getDataLayout();
----------------
I'd s/`LoadOp`/`Addr`/, since very soon we transform it to a value that isn't the load operand.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:493
+      cast<PointerType>(LoadOp->getType())->getElementType());
+  uint32_t MaxNumUses = 8;
+
----------------
Sorry did not notice this earlier, but this should be a `cl::opt`.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:502
+  while (LoadOp->getType() != PtrInt8Ty) {
+    auto *BC = dyn_cast<BitCastInst>(LoadOp);
+    if (!BC)
----------------
I'd like a bound on this loop too.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:516
+    IntrinsicInst *II = dyn_cast<IntrinsicInst>(U);
+    // If there are invariant.end instructions, the load maybe
+    // non-invariant. The only use of an invariant.start instruction is
----------------
s/load maybe/load maybe is/


================
Comment at: lib/Transforms/Scalar/LICM.cpp:527
+    if ((LocSizeInBits <=
+         cast<ConstantInt>(II->getArgOperand(0))->getSExtValue() * 8) &&
+        DT->dominates(II->getParent(), CurLoop->getLoopPreheader()))
----------------
I'd extract out `cast<ConstantInt>(II->getArgOperand(0))->getSExtValue() * 8` as `InvariantSizeInBits` or something like that.


================
Comment at: test/Transforms/LICM/hoisting.ll:194
+; CHECK-LABEL: @test_fence1
+; CHECK-LABEL: entry
+; CHECK: invariant.start
----------------
Another related example -- there is no explicit invariant_end but the return value of invariant_start is escaped (e.g. passed to an opaque function) so there may possibly be an invariant_end somewhere we can't see.


https://reviews.llvm.org/D29331