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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

You may have though about `ArrayRef<const Loop*> getLoops() const` instead of `const ArrayRef<Loop*> getLoops() const`.

However, I think the `const` qualifier on the method applies to the analysis result, not the loops it holds (which are the analysis result of `LoopInfo`). The loops are identity objects: modifying them does not change which loops they represent. To create a `LoopNest`, one requires a non-const `Loop*` object anyway.

A language-layer might say that when requiring a non-const `Loop*` at construction, we already know that the `Loop*` is stored in modifyable memory (not a const global), hence can be modified.



================
Comment at: llvm/include/llvm/Analysis/LoopNestAnalysis.h:64
+  /// Return the innermost loop in the loop nest if the nest has only one
+  /// innermost loop, and a nullptr otherwise.
+  const Loop *getInnermostLoop() const {
----------------
[suggestion] Add a remark that the outermost and the returned are not necessarily perfectly nested.


================
Comment at: llvm/include/llvm/Analysis/LoopNestAnalysis.h:84-87
+  const Loop *getLoop(unsigned Index) const {
+    assert(Index < Loops.size() && "Index is out of bounds");
+    return Loops[Index];
+  }
----------------
[serious] This accessor makes no sense. The same Loop* (non-const) can be obtained from a `const LoopNest` using `getLoops()[Index]`.

`LoopNest` does not own the `Loop`s and therefore has no control over its const-ness. Just use a single `Loop *getLoop(unsigned) const` accessor. `const` on `LoopNest` refers to the constness of the analysis result. The `Loop`s themselves are identity objects: after e.g. removing a BB out of its lists (e.g. because it is dead), it still represents the same loop. Same applies to `getOutermostLoop()`.

To make the accessor useful, also provide a `getNumLoops()` member (or just remove all of it and require users to use `getLoops()`).


================
Comment at: llvm/include/llvm/Analysis/LoopNestAnalysis.h:94
+  /// Get the loops in the nest.
+  const ArrayRef<Loop*> getLoops() const { return Loops; }
+
----------------
[style] `const ArrayRef` is useless. It's an r-value/temporary.


================
Comment at: llvm/include/llvm/Analysis/LoopNestAnalysis.h:140-141
+  /// false otherwise.
+  static bool checkLoopsStructure(const Loop &OuterLoop, const Loop &InnerLoop,
+                                  ScalarEvolution &SE);
+
----------------
[style] Since it is private, move it as a free function into the .cpp file. No need to expose it in a header file.


================
Comment at: llvm/include/llvm/Analysis/LoopNestAnalysis.h:145
+  const unsigned MaxPerfectDepth; // maximum perfect nesting depth level.
+  LoopVectorTy Loops; // the loops in the nest (in breath first order).
+};
----------------
[typo] brea**d**th 


================
Comment at: llvm/lib/Analysis/LoopNestAnalysis.cpp:32-33
+  : MaxPerfectDepth(getMaxPerfectDepth(Root, SE)) {
+  for (Loop *L : breadth_first(&Root))
+    Loops.push_back(L);
+}
----------------
I still think storing all a list of all loops doesn't have a lot of benefits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68789