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

I'm not sure this is correct -- what if you had:

  int* ptr = ...;
  for (...) { // runs only one iteration
    *ptr = 20;
    invariant_start(ptr); // invariant, but only after we've written 20 to *ptr
    int val = *ptr;
  }

I think this patch will LICM the load to outside the loop incorrectly.  I suspect we can at most hoist up to right after the `invariant_start` call.



================
Comment at: lib/Transforms/Scalar/LICM.cpp:504
+  while (isa<BitCastInst>(LoadOp)) {
+    if (isInvariantStartOperandType())
+      break;
----------------
This is very hard to read, not the least because you're implicitly using `LoadOp`. :)

Can you instead phrase this more straightforwardly, as:

```
auto *Int8PtrTy = ...
while (isa<bitcast>(...)) {
  if (LoadOp->getType() == Int8PtrTy)
    break;
   ...
}
```



================
Comment at: lib/Transforms/Scalar/LICM.cpp:529
+        // non-invariant. The only use of an invariant.start instruction is
+        // within its corresponding invariant.end instruction.
+        !II->getNumUses() && DT->dominates(II, LI))
----------------
Don't you have to check the size?


https://reviews.llvm.org/D29331