[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've left a first pass of comments. I would recommend splitting off the parts that are not directly required for getBounds, like getGuard, as those seem orthogonal. It seems there are a lot of checks that ScalarEvolution can already take care of and IMO it would be better to require ScalarEvolution for those things.



================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:609
+  /// else return nullptr.
+  std::unique_ptr<LoopBounds> getBounds(ScalarEvolution *SE = nullptr) const;
+
----------------
Do we need dynamic allocations here? The LoopBounds struct is small and trivially movable/copyable, so just returning it as a value would be simpler and probably faster.


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:639
+  /// each time through the loop.
+  bool isCanonical(ScalarEvolution *SE = nullptr) const;
+
----------------
Isn't that unnecessarily strict?


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:23
 #include "llvm/Analysis/LoopIterator.h"
+#include "llvm/Analysis/PostDominators.h"
+#include "llvm/Analysis/ScalarEvolution.h"
----------------
Looks unused?


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


================
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()))
----------------
I'm not sure we should use SimplifyInstruction here. Ideally everything should be already simplified?


================
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())
----------------
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?


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:221
+  if (C1 != nullptr && C2 != nullptr) {
+    if ((C1->getSExtValue() - C2->getSExtValue()) == 1)
+      return true;
----------------
This is treating the values as signed, could they not be unsigned as well? Also, you would need to check if the type size is <= 64 bit, otherwise it will assert.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:225
+
+  // V1 = add V2, 1 or V1 = add 1, V2
+  Instruction *I1 = dyn_cast<Instruction>(&V1);
----------------
AFAIK, add 1, V2, should have been canonicalized to add V2, 1 already.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:283
+Direction Loop::LoopBounds::getDirectionFromConstantBounds() const {
+  // Strip all ZExtInst or SExtInst from V
+  auto stripExt = [](Value *V) {
----------------
again, getting this would be trivial from an AddRecExpr & SE?


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


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:669
+
+  Instruction *StepInst =
+      dyn_cast<Instruction>(AuxIndVar.getIncomingValueForBlock(Latch));
----------------
Wouldn't it be enough to just check if it is an SCEVAddRecEpxr?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60565