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

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:478
+    if (!SE->isLoopInvariant(Op, L) ||
+        !isSafeToExpandAt(Op, Preheader->getTerminator(), *SE))
       return Use;
----------------
apilipenko wrote:
> 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.
You are correct, and I'd completely missed that when writing.  Will do!


================
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.  
----------------
apilipenko wrote:
> 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.
No we can't.  That's the entire point of not checking.

The basic structure here is that isSafeToExpand guarantees expansion is safe, but a false return value can still be safe to expand.  In this case, we use information about dominance of the original conditions in the IR to ensure safety of these two.


================
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)) {
----------------
reames wrote:
> apilipenko wrote:
> > 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.
> No we can't.  That's the entire point of not checking.
> 
> The basic structure here is that isSafeToExpand guarantees expansion is safe, but a false return value can still be safe to expand.  In this case, we use information about dominance of the original conditions in the IR to ensure safety of these two.
I thought about it, but explaining the NFC part without context seemed challenging.  So I left them together.


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:1003
 
+  printLoop(*L, dbgs());
   return Changed;
----------------
apilipenko wrote:
> Debug printing.
Oops, good catch, thanks.


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

https://reviews.llvm.org/D60093