[PATCH] D138232: [NFC] Refactor loop peeling code for calculating phi invariance.

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

I'm generally fine with this refactoring. Some style comments, the most important one being regarding using a constant to represent infinity instead of `None`.

For test -- LGTM, you can commit it separately, it doesn't require this NFC to go with it.



================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:107
+// As a loop is peeled, it may be the case that Phi nodes become
+// deterministic (ie, known because there is only one choice).
+// For example, consider the following function:
----------------
"Deterministic" isn't the best word here, because Phi value is always determined by its incoming block. How about "loop-invariant"?


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:118
+//       else
+//         g(2);
+//       g(x);
----------------
If it wasn't your intention, please change condition `i > 10` here, otherwise we can say that it's (maybe) also profitable to peel off 10 iterations to get rid of this check (and it might be even more important than what are you describing).


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:167
+public:
+  PhiAnalyzer(Loop *L, unsigned MaxIterations);
+
----------------
Can it be `const Loop *` here and in other places? Looks like this thing isn't intended to modify the code.


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:169
+
+  // Calculate the allowable maximum number of iterations of the loop to peel
+  // such that phi instructions become determined
----------------
"allowable maximum" -> "sufficient minimum"? We could off course peel more, just don't see a reason to.


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:175
+  static Loop &checkLoop(Loop *);
+  static BasicBlock &getBackEdge(Loop *);
+
----------------
Do we really need that, provided that there is L->getLatch()?


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:179
+  unsigned addOne(unsigned N) const {
+    return N >= MaxIterations ? Infinity : N + 1;
+  }
----------------
Can it return `Infinity + 1` if `MaxIterations > Infinity`? Or, if it's impossible, please assert it here explicitly.


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:187
+  Loop &L;
+  BasicBlock &BackEdge;
+
----------------
Doesn't really look like it's worth storing it separately, it's cheaply claimed from loop when needed.


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:191
+
+  // Designates that a Phi is estimated to become invariant after an
+  // "infinite" number of loop iterations (i.e. only may become an
----------------
Does this mean that this Phi provably never becomes invariant, or it can also mean that we haven't found this number (as it was in the original code)?

I think the original code was structured better here for 2 reasons:
- Having a constant to represent infinity is potentially error-prone. Who knows what kind of math people are going to do with it? Using `None` for it is just more clear.
- Old code used `None` for unknowns in fact, and this code claims to know it's infinity.

I'd suggest returning `Optional<unsigned>` as a safer structure for this.


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:197
+  // Map of Values to number of iterations to invariance
+  std::map<const Value *, unsigned> IterationsToInvariance;
+};
----------------
Please use `DenseMap` unless there are strong reasons to choose a `std::` container.


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:200
+
+inline Loop &PhiAnalyzer::checkLoop(Loop *L) {
+  assert(L && canPeel(L) && "Loop is not suitable for peeling.");
----------------
I don't think you ever expect nullptr loops to be a normal input. Change to `const Loop &`?


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:210
+
+inline PhiAnalyzer::PhiAnalyzer(Loop *LP, unsigned M)
+    : L(checkLoop(LP)), BackEdge(getBackEdge(LP)), MaxIterations(M),
----------------
`LP -> L`, `M -> MaxIterations`?


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

https://reviews.llvm.org/D138232