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

In https://reviews.llvm.org/D29331#661997, @sanjoy wrote:

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


Thanks for catching that.  I dont think we have a mechanism currently to hoist upto a specific instruction. LICM uses `canSinkOrHoistInst` to hoist the load to just before the terminator of preheader of the current loop `CurLoop`. Behaviour of sinking to end of loop won't be affected by this patch.

So, I think we can fix this problem by add one more condition for hoisting such loads:  the dominating `invariant.start`'s BB should be outside `CurLoop`. I have a patch for this and it catches the bug above (and from reading LICM code, nested loops with invariant.start would work as well).



================
Comment at: lib/Transforms/Scalar/LICM.cpp:504
+  while (isa<BitCastInst>(LoadOp)) {
+    if (isInvariantStartOperandType())
+      break;
----------------
sanjoy wrote:
> 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;
>    ...
> }
> ```
> 
Yes, that implicit `LoadOp` makes it pretty hard. I used a lambda because of the same logic at two places. I think the `if check` as you've written above is fine even with it will be duplicated. 


================
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))
----------------
sanjoy wrote:
> Don't you have to check the size?
yes. The implementation of invariant.start has that possibility of specifying a diffferent size. I'll add that. 


https://reviews.llvm.org/D29331