[PATCH] D68789: [LoopNest]: Analysis to discover properties of a loop nest.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

I'd only store the list of perfectly nested loops that are have the analysis loop as outermost loop in the analysis results. This should be the most expensive analysis that might be worth caching/preserving between passes.



================
Comment at: llvm/include/llvm/Analysis/LoopNestAnalysis.h:34-35
+
+  /// Return true if the given loops \p OuterLoop and \p InnerLoop are
+  /// perfectly nested with respect to each other, and false otherwise.
+  static bool arePerfectlyNested(const Loop &OuterLoop, const Loop &InnerLoop,
----------------
Could you clarify whether this allows other perfectly nested loops between outer and inner? Eg.
```
for outer:
   for middle:
      for inner:
```



================
Comment at: llvm/include/llvm/Analysis/LoopNestAnalysis.h:36-37
+  /// perfectly nested with respect to each other, and false otherwise.
+  static bool arePerfectlyNested(const Loop &OuterLoop, const Loop &InnerLoop,
+                                 ScalarEvolution &SE);
+
----------------
[suggestion] Did you consider moving the static methods to LoopUtils?


================
Comment at: llvm/include/llvm/Analysis/LoopNestAnalysis.h:62
+  /// Return true if the loop nest rooted by loop \p Root is perfect.
+  static bool isPerfectNest(const Loop &Root, ScalarEvolution &SE);
+
----------------
[serious] As already mentioned, I don't see a use case for this. Transformations should not have more requirements than necessary, including the content of the innermost loop of a perfect loop nest, and I don't see how the presence of a nested loop should be a requirement of any transformation.

Please either try to convince me otherwise or remove it to avoid it being used accidentally.

Same applies to `getNestDepth`.


================
Comment at: llvm/include/llvm/Analysis/LoopNestAnalysis.h:67-68
+  Loop &getOutermostLoop() {
+    return const_cast<Loop &>(
+        static_cast<const LoopNest &>(*this).getOutermostLoop());
+  }
----------------
[subjective] I'd prefer `*Loops.front()` over `const_cast`.


================
Comment at: llvm/include/llvm/Analysis/LoopNestAnalysis.h:102
+  /// Get the loops in the nest.
+  const LoopVector &getLoops() const { return Loops; }
+
----------------
[suggestion] Return a `ArrayRef<Loop*>`. Returning `LoopVector` exposes more implementation details than necessary.


================
Comment at: llvm/include/llvm/Analysis/LoopNestAnalysis.h:116
+
+  /// Return the loop nest depth (e.g. the loop depth of the 'deepest' loop)
+  /// For example given the loop nest:
----------------
[typo] "i.e." (that is) instead of "e.g." (for example)


================
Comment at: llvm/lib/Analysis/LoopNestAnalysis.cpp:30-34
+LoopNest::LoopNest(Loop &Root, ScalarEvolution &SE)
+  : MaxPerfectDepth(getMaxPerfectDepth(Root, SE)) {
+  for (Loop *L : breadth_first(&Root))
+    Loops.push_back(L);
+}
----------------
[discussion] The analysis looks like a cache for the the perfect loop depth and the loops in breadth-first order. Everything else is computed on-the-fly. Does this justify being a pass?  

[serious] I also don't see the advantage of storing the loop list over iterating the LoopInfo structure itself when needed. Is is for speed? Also note that loop analysis might be instantiated for every loop, each storing its own list.


================
Comment at: llvm/lib/Analysis/LoopNestAnalysis.cpp:91
+  //  - a phi node, a cast or a branch
+  auto containSafeInstructions = [&](const BasicBlock &BB) {
+    return llvm::all_of(BB, [&](const Instruction &I) {
----------------
[name] `containsOnlySafeInstructions`


================
Comment at: llvm/lib/Analysis/LoopNestAnalysis.cpp:217-221
+  const BasicBlock *OuterLoopHeader = OuterLoop.getHeader();
+  const BasicBlock *OuterLoopLatch = OuterLoop.getLoopLatch();
+  const BasicBlock *InnerLoopPreHeader = InnerLoop.getLoopPreheader();
+  const BasicBlock *InnerLoopLatch = InnerLoop.getLoopLatch();
+  const BasicBlock *InnerLoopExit = InnerLoop.getExitBlock();
----------------
[style] The LLVM code base does not use `const` for function-local variables.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68789