[PATCH] D60093: [LoopPredication] Allow predication of loop invariant computations

[Artur Pilipenko via Phabricator via llvm-commits]

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

Looks good overall, a couple of comments inline.



================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:478
+    if (!SE->isLoopInvariant(Op, L) ||
+        !isSafeToExpandAt(Op, Preheader->getTerminator(), *SE))
       return Use;
----------------
Am I right that this is not specific to the change you make and just a fix for an existing bug? If so,  can be integrated separately with a test demonstrating the problem.


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:528-530
+  // Subtlety: We need all the values to be *invariant* across all iterations,
+  // but we only need to check expansion safety for those which *aren't*
+  // already guaranteed to dominate the guard.  
----------------
Can we assert isSafeToExpandAt for the values we don't check?


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:528-547
+  // Subtlety: We need all the values to be *invariant* across all iterations,
+  // but we only need to check expansion safety for those which *aren't*
+  // already guaranteed to dominate the guard.  
+  if (!isLoopInvariantValue(GuardStart) ||
+      !isLoopInvariantValue(GuardLimit) ||
+      !isLoopInvariantValue(LatchStart) ||
+      !isLoopInvariantValue(LatchLimit)) {
----------------
apilipenko wrote:
> Can we assert isSafeToExpandAt for the values we don't check?
These changes look correct to me, but there are actually a couple of NFC refactorings (replacing CanExpand(RHS) with CanExpand(LatchStart), inlining CanExpand, removing isSafeToExpandAt checks for GuardStart, GuardLimit) mixed with a functional change to use isLoopInvariantValue instead of SE->isLoopInvariant. You might want to split these when integrating to make bisecting easier.


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:1003
 
+  printLoop(*L, dbgs());
   return Changed;
----------------
Debug printing.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60093/new/

https://reviews.llvm.org/D60093