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

[Whitney via Phabricator via llvm-commits]

Whitney marked 6 inline comments as done.
Whitney added inline comments.


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:639
+  /// each time through the loop.
+  bool isCanonical(ScalarEvolution *SE = nullptr) const;
+
----------------
fhahn wrote:
> Isn't that unnecessarily strict?
There exists getCanonicalInductionVariable() which returns the first PHINode which starts at 0 and increments by one each time through the loop. I would like to use the same terminology here.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:23
 #include "llvm/Analysis/LoopIterator.h"
+#include "llvm/Analysis/PostDominators.h"
+#include "llvm/Analysis/ScalarEvolution.h"
----------------
fhahn wrote:
> Looks unused?
That's right, I will remove this include.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:172
+  ConstantInt *C2 = dyn_cast<ConstantInt>(&V2);
+  if (C1 != nullptr && C2 != nullptr) {
+    if (C1->getSExtValue() == C2->getSExtValue())
----------------
fhahn wrote:
> LLVM coding convention is not to check for nullptr explicitly, just use if (C1 && C2) (here and below)
Didn't know this coding convention, will remove the nullptr checks.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:191
+  // Simplify(V1) == Simplify(V2)
+  if (Instruction *I1 = dyn_cast<Instruction>(&V1))
+    if (Value *V = SimplifyInstruction(I1, I1->getModule()->getDataLayout()))
----------------
fhahn wrote:
> I'm not sure we should use SimplifyInstruction here. Ideally everything should be already simplified?
I agree that ideally everything should be already simplified, but I encounter cases which the instructions are not simplified, and I don't think we can assume the instructions are simplified or not, as getGuard() can be called in any point in time.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:213
+/// Return true if V1 == V2 + 1
+static bool diffOne(Value &V1, Value &V2, ScalarEvolution *SE = nullptr) {
+  if (V1.getType() != V2.getType())
----------------
fhahn wrote:
> I guess the manual handling might be faster in some cases, but this seems like a thing SE should handle for us. Would it be possible to just require it and use it?
The ideal here is trying to get these loop information without the requirement of SE, but optionally takes SE for better result.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:420
+  // First operand of CI should be the StepInst
+  if (isa<Instruction>(CI->getOperand(0)))
+    return CI;
----------------
fhahn wrote:
> could just be return dyn_cast<Instruction>(CI->getOperand(0))?
agree


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60565