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

Minor comments inline.



================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:109
+    //   If %y is a Phi from the loop header, Depth(%x) = Depth(%y) + 1.
+    //   Otherwise, Depth(%x) = 0.
+    std::function<unsigned(PHINode *)> CalculateInvariantDepth =
----------------
I think the logic may be more understandable if instead of returning `0` for a loop-varying-non-header-phi `%x`, you return `DepthInfinity` where `DepthInfinity` is an alias for `UINT_MAX`.  Returning `0` in these cases seems a bit odd, since given your scheme I'd expect a dept of `0` meaning that the value is loop invariant, not that it is "hopelessly loop invariant".


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:111
+    std::function<unsigned(PHINode *)> CalculateInvariantDepth =
+        [&](PHINode *Phi) {
+      // If we already know the answer, take it from the map.
----------------
I think this is too large to be a lambda -- can you please extract it out into a static helper?


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:126
+        Depth = 1u;
+      else if (isa<PHINode>(Input)) {
+        PHINode *IncPhi = cast<PHINode>(Input);
----------------
Use `else if (PHINode *IncPhi = dyn_cast<PHINode>(Input)) {`


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:137
       }
+
+      // If we found that this Phi lies in an invariant chain, update the map.
----------------
One TODO here would be to consider some minor binary operations as well, like:

```
A = PHI(0, C)
B = PHI(0, 5)
C = B + 1
```

Ideally the depth of `A` should be `2`, not `0`.


https://reviews.llvm.org/D31613