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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Are you planning to add more users of this interface?



================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:541
 
+  /// Example:
+  /// for (int i = lb; i < ub; i+=step)
----------------
Could you add a short description in addition to the example?


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:550
+  /// loop:
+  ///   i1 = phi[{lb, preheader}, {i2, latch}]
+  ///   <loop body>
----------------
[suggestion] Could you name this to something other than `i1`? At first glance in the results below, I though it is a type.


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:576
+        : L(Loop), Init(I), StepInst(S), CmpInst(C), SE(SE) {}
+    /// Get the initial value of the loop induction variable.
+    Value &getInitialIVValue() const { return Init; }
----------------
[style] Could you add an empty line before each doxygen comment?


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:164
 
+/// Return true if V1 and V2 have the same value
+static bool isEqual(Value &V1, Value &V2, ScalarEvolution &SE) {
----------------
SCEV values are uniqued, so in that sense V1 and V2 are equal if, and only if, `&V1==&V2`. Could you add a clarification to the comment and/or function name that this `isEqual` ignores bit width?


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:322-334
+/// beforeloop:
+///   guardcmp = (lb < ub)
+///   if (guardcmp) goto preheader; else goto afterloop
+/// preheader:
+/// loop:
+///   i1 = phi[{lb, preheader}, {i2, latch}]
+///   <loop body>
----------------
[style] Please wrap code inside [[ https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments | `\code ... \endcode`  ]]


================
Comment at: llvm/unittests/Analysis/LoopInfoTest.cpp:236-254
+      "define void @foo(i32* %A, i32 %ub) {\n"
+      "entry:\n"
+      "  %guardcmp = icmp slt i32 0, %ub\n"
+      "  br i1 %guardcmp, label %for.preheader, label %for.end\n"
+      "for.preheader:\n"
+      "  br label %for.body\n"
+      "for.body:\n"
----------------
[suggestion] This is easier to format using C++11 raw strings. See `unittests/Transforms/Utils/UnrollLoopTest.cpp`.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60565