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

[Ettore Tiotto via Phabricator via llvm-commits]

etiotto added inline comments.


================
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];
+  }
----------------
Meinersbur wrote:
> [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()`).
Removed the member functions that return a const Loop. Added `getNumLoops()`.


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


================
Comment at: llvm/lib/Analysis/LoopNestAnalysis.cpp:32-33
+  : MaxPerfectDepth(getMaxPerfectDepth(Root, SE)) {
+  for (Loop *L : breadth_first(&Root))
+    Loops.push_back(L);
+}
----------------
Meinersbur wrote:
> I still think storing all a list of all loops doesn't have a lot of benefits.
I think is useful to the transformation to know which loops are in the loop nest. It is used in several places. For example is handy tp have a list of loops in the nest to check whether they are all in simplified form. It is also used to determine the nst depth, etc... 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68789