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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

I still think there is some duplication with InductionDescriptor that could be unified, which would also make it easier to use this new utility in places that currently use InductionDescriptor.

Some comments inline, also InductionDescriptor already stores & provides accessors for InitialIVValue, StepValue (although just the SCEV/ConstantInt - is the generic value actually what you need?) and StepInst. With my earlier comment, I was going more in the direction of using IVDescriptor, instead of duplicating the fields here and using the existing functions to detect induction PHIs.



================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:61
 class PHINode;
+class PostDominatorTree;
+class ScalarEvolution;
----------------
Unused?


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:636
+    /// Get the direction of the loop.
+    Direction getDirection() const;
+
----------------
This is basically the same as InductionDescriptor::getConsecutiveDirection


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:657
+    // The final value of the loop induction variable
+    Value &FinalIVValue;
+
----------------
I am not sure what this means exactly. Do you have checks to exclude loops with multiple exit blocks?

 Also, I am not sure what you intend on using this value for. Wouldn't it be less fragile to use SCEV to reason about the number of iterations? You can also evaluate the AddRec expression of the induction PHI at the final loop iteration.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:319
+
+PHINode *Loop::getInductionVariable(ScalarEvolution &SE) const {
+  if (!isLoopSimplifyForm())
----------------
Isn't the main part of this function the same as InductionDescriptor::isInductionPHI() ? http://llvm.org/doxygen/classllvm_1_1InductionDescriptor.html#a1974b8e8a8bae482a772ebfab1e9c794


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60565