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

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:508
+  // Avoid traversing for LoadOperand with high number of users.
+  if (LoadOp->getNumUses() > MaxNumUses)
+    return false;
----------------
sanjoy wrote:
> You're not saving any time by this, since `getNumUses()` is O(# uses).
> 
> I think you need a `if (UsesSeen++ > MaxNumUses) return false;` type thing in the loop below.
thanks. done.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:535
+    // load out of a loop that contains this dominating invariant.start
+    if (DT->dominates(II, LI) && !CurLoop->contains(II->getParent()))
+      return true;
----------------
sanjoy wrote:
> I think just checking `DT->dominates(II, LoopPreHeader)` is sufficient here.
that's right. I missed that the `LoopPreheader` is guaranteed to exist in loops that have LICM done on them (all these loops are in `LCSSA` form).


https://reviews.llvm.org/D29331