[PATCH] D60565: [LOOPINFO] Extend Loop object to add utilities to get the loop bounds, step, induction variable, and guard branch.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Some initial comments from my side.



================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:584
+    /// - the LHS of the latch comparison is StepInst
+    ICmpInst::Predicate getCanonicalPredicate() const;
+    /// An enum for the direction of the loop
----------------
This definition seems to lose given that there are various corner cases, e.g., wrapping induction variables.


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:620
+
+  /// Return the loop induction variable if found, else return nullptr.
+  /// An instruction is considered as induction variable if
----------------
"The" loop induction variable, or "any" loop induction variable. There could easily be two or more.


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:635
+  bool isAuxiliaryInductionVariable(PHINode &AuxIndVar,
+                                    ScalarEvolution *SE = nullptr) const;
+
----------------
Why is it called auxiliary and what happens if you call it with the only induction variable?


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:199
+
+  // Use SE if given
+  if (!SE)
----------------
I would have assumed the SE check is performed first if SE is available. 


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:291
+  if (Step) {
+    if (Step->isNegative()) {
+      // i += negative --> i -= positive --> Decreasing
----------------
Stripping `zext` from `zext i8 -1 to i32` will result in a negative value while the original step is not. Here and in other places, I'm not convinced that "stripping" is generally sound.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:314
+      return Direction::Increasing;
+    // initial_value > final_value --> Decreasing
+    else
----------------
This reasoning is not valid if the induction variable can overflow.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:651
+  // No uses outside of the loop
+  for (auto U : AuxIndVar.users())
+    if (auto I = dyn_cast<Instruction>(U))
----------------
What is the type of `U` here? It is not clear and you might end up calling a copy constructor.
Similar below.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60565