[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 more comments, though I haven't reviewed all of the code, sorry.



================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:186
+void Loop::LoopBounds::setInitialIVValue(PHINode &IndVar) {
+  InductionDescriptor IndDesc;
+  assert(InductionDescriptor::isInductionPHI(&IndVar, &L, &SE, IndDesc) &&
----------------
Could you explain this to me please. I see `IndDesc` and `IndVar` but outside the `assert`, the former is not initialized, the latter not used. I'm confused.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:322
+  BasicBlock *Succ =
+      nullptr; // The successor of GuardBB which 'flows' into the loop preheader
+  SmallPtrSet<BasicBlock *, 4> Visited;
----------------
Could you "clang-format" the code if possible. Even if not, the newline to accommodate a comment in the same line seems odd, just put it in the line before.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:338
+  } while (GuardBI && !GuardBI->isConditional());
+  if (!GuardBI || !GuardBI->isConditional())
+    return nullptr;
----------------
Newline missing.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:401
+  else
+    assert(false);
+  Value &LatchFinalValue = Bound->getFinalIVValue();
----------------
`llvm_unreachable("SOME HELPFUL TXT")` or sth else that meaningful (so no `assert(false)`)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60565