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

[Ettore Tiotto via Phabricator via llvm-commits]

etiotto marked 17 inline comments as done.
etiotto added inline comments.


================
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,
----------------
Meinersbur wrote:
> Could you clarify whether this allows other perfectly nested loops between outer and inner? Eg.
> ```
> for outer:
>    for middle:
>       for inner:
> ```
> 
Added example in a comment. 


================
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);
+
----------------
Meinersbur wrote:
> [suggestion] Did you consider moving the static methods to LoopUtils?
Yes, I think is nice to keep all the code related to the loop nest analysis grouped together in this class.


================
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);
+
----------------
Meinersbur wrote:
> [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`.
OK removing these 2 static functions


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


================
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);
+}
----------------
Meinersbur wrote:
> [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.
OK I will change this from an analysis pass to an utility class in the next revision. Perhaps over time, as we find the need to add more functionality to this class, we can revisit this point and determine whether it makes more sense to have an analysis pass.


================
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();
----------------
Meinersbur wrote:
> [style] The LLVM code base does not use `const` for function-local variables.
I am trying to ensure the basic blocks are not mutated in the function. I am not sure what was the rationale (in the LLVM code base) for not wanting to const qualify function-local variables if they are intended to remain immutable ... perhaps the thinking was that the original author of the code would not make a mistake and modify something that was intended to stay immutable, but I think is safer to const qualify variables to prevent unintended changes down the road.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68789