[PATCH] D31613: [LoopPeeling] Get rid of Phis that become invariant after N steps

[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/Utils/LoopUnrollPeel.cpp:50
+// Designates infinite invariant depth of a Phi node.
+static const unsigned InfiniteDepth = UINT_MAX;
+
----------------
s/InfiniteDepth/InfiniteInvariantDepth/


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:84
+//         %y = phi(0, 5),
+//         %a = %y + 1.
+static unsigned calculateInvDepth(PHINode *Phi, Loop *L,
----------------
I should have mentioned this in the earlier revision, but do you think `computeIterationsToInvariance` is a better name for this function?  `Depth` does not clarify what we're computing here very much.

If you do this, please also rename `InfiniteDepth` (this suggestion also invalidates the "expand Inv to Invariant" suggestion).


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:85
+//         %a = %y + 1.
+static unsigned calculateInvDepth(PHINode *Phi, Loop *L,
+                                  SmallDenseMap<PHINode *, unsigned> &Depths) {
----------------
I'd expand `Inv` to `Invariant`.


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:94
+
+  BasicBlock *BackEdge = L->getLoopLatch();
+  assert(BackEdge && "Loop is not in simplified form?");
----------------
This is a suggestion, feel free to ignore:

`getLoopLatch` isn't super-cheap (it re-discovers the latch every time).  How about passing the latch explicitly to this function?


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:109
+      return InfiniteDepth;
+    // Try to estimate the depth of input. If it is finite, our phi's
+    // depth is greater by 1 according to definition.
----------------
s/estimate/compute an upper bound on/


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:138
+  //
+  // Given %x = phi <Inputs from above the loop>, ..., [%y, %back.edge].
+  //   If %y is a loop invariant, then Depth(%x) = 1.
----------------
I don't think there is a need to explain the algorithm here (you should do that on the implementation, as you've already done).  Instead renaming the helper to `computeIterationsToInvariance` will make the logic here more obvious.


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:149
+  // First, check that we can peel at least one iteration.
+  if (2 * LoopSize <= UP.Threshold) {
+    // Store pre-calculated values of Depth here.
----------------
Why did you change the condition to `2 * LoopSize <= UP.Threshold`?


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:165
+      DesiredPeelCount = std::min(DesiredPeelCount, MaxPeelCount);
+      assert(DesiredPeelCount > 0 && "Wrong loop size estimation?");
+      DEBUG(dbgs() << "Peel " << DesiredPeelCount << " iteration(s) to get rid"
----------------
Can `UP.Threshold` be `0` (say it was directly set by the user by `UnrollThreshold`)?


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:168
+                   << " of some Phis forming an invariant chain.\n");
+      UP.PeelCount = DesiredPeelCount;
       return;
----------------
I don't know the rest of the code well enough to be conclusive on this, but should this be `UP.PeelCount = std::max(UP.PeelCount, DesiredPeelCount);`?  That is, if we've earlier decided, due to some other reasons, that peeling for 5 iterations is a good idea, and here we decide that peeling for only 3 iterations is a good idea, perhaps we should still peel for 5 iterations?

Actually, even if `UP.PeelCount` > `DesiredPeelCount` is impossible, I think `UP.PeelCount = std::max(UP.PeelCount, DesiredPeelCount);` seems cleaner.


================
Comment at: test/Transforms/LoopUnroll/peel-loop-not-forced.ll:60
+define i32 @invariant_backedge_3(i32 %a, i32 %b) {
+; This loop should be peeled trice because it has a Phi which becomes invariant
+; starting from 4th iteration.
----------------
s/trice/thrice/


================
Comment at: test/Transforms/LoopUnroll/peel-loop-not-forced.ll:92
+define i32 @invariant_backedge_limited_by_size(i32 %a, i32 %b) {
+; This loop should normally be peeled trice because it has a Phi which becomes
+; invariant starting from 4th iteration, but the size of the loop only allows
----------------
s/trice/thrice/


https://reviews.llvm.org/D31613