[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 requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

Comments inline.



================
Comment at: lib/Transforms/Scalar/LICM.cpp:505
+    LoadOp = cast<BitCastInst>(LoadOp)->getOperand(0);
+  }
+
----------------
I'd move the `LoadOp->getType() != PtrInt8Ty` check right after the while loop, since they're related.

Another way to write this would be:

```
while (LoadOp->getType() != PtrInt8Ty) {
  auto *BC = dyn_cast<BitCastInst>(LoadOp);
  if (!BC)
    return false;
  LoadOp = BC->getOperand(0);
}

// No need to check the type again
```



================
Comment at: lib/Transforms/Scalar/LICM.cpp:508
+  // Avoid traversing for LoadOperand with high number of users.
+  if (LoadOp->getNumUses() > MaxNumUses)
+    return false;
----------------
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.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:525
+    if (!II || II->getIntrinsicID() != Intrinsic::invariant_start ||
+        II->getNumUses())
+      continue;
----------------
Don't call `getNumUses` here, since it is O(#uses).  Instead, use `hasNUsesOrMore(1)`.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:530
+    if (LocSizeInBits !=
+        cast<ConstantInt>(II->getArgOperand(0))->getSExtValue() * 8)
+      continue;
----------------
Can be `LocSizeInBits <= `.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:533
+
+    // The invariantstart should dominate the load, and we should not hoist the
+    // load out of a loop that contains this dominating invariant.start
----------------
s/`invariantstart`/`invariant_start`/


================
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;
----------------
I think just checking `DT->dominates(II, LoopPreHeader)` is sufficient here.


https://reviews.llvm.org/D29331