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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

[serious] I think this pass should determine the maximum set of loops that are perfectly nested, as `getMaxPerfectDepth` returns. This would make

  for (int i)
    for (int j) {
      preStmt();
      for (int k) ...
      postStmt();
    }

a perfectly nested loop nest of depth 2 with a body that contains a loop. In fact, any loop would be perfectly nested, but possibly just with depth one. This would avoid different analysis result between the above loop nest and

  for (int i)
    for (int j) {
      OutlinedBody(i,j);
    }

[serious] There could be irreducible control flow between two loops, that needs to be ruled-out.



================
Comment at: llvm/include/llvm/Analysis/LoopNestAnalysis.h:65
+  /// Return the outermost loop in the loop nest.
+  const Loop &getOuterMostLoop() const { return *Loops.front(); }
+  Loop &getOuterMostLoop() {
----------------
[nit]  [[ https://en.wiktionary.org/wiki/outermost | outermost ]] is a proper english word, so I suggest: `getOutermostLoop()`


================
Comment at: llvm/include/llvm/Analysis/LoopNestAnalysis.h:77
+
+    // The loops in the 'Loops' vector have been collected in breath first
+    // order, therefore if the last 2 loops in it have the same nesting depth
----------------
[typo] brea**d**th


================
Comment at: llvm/include/llvm/Analysis/LoopNestAnalysis.h:124
+  /// Return true if the loop nest is perfect and false otherwise.
+  bool isPerfect() const { return MaxPerfectDepth == getNestDepth(); }
+
----------------
[discussion] I understand "perfectly nested" as a property of a set of loops. That is, in
```
  for (int i)
    for (int j) {
      preStmt();
      for (int k) ...
      postStmt();
    }
```
I'd understand the loop `i` and `j` to be a perfect loop nest. What code is in the body does not matter.


================
Comment at: llvm/lib/Analysis/LoopNestAnalysis.cpp:23
+#define DEBUG_TYPE "loopnest"
+static const char *VerboseDebug = DEBUG_TYPE "-verbose";
+
----------------
Is this an established pattern? Never seen this before, but looks practical. The other pattern is `VerboseFusionDebugging`. Maybe we should agree on one patten.


================
Comment at: llvm/lib/Analysis/LoopNestAnalysis.cpp:70
+  const CmpInst *OuterLoopLatchCmp = dyn_cast<CmpInst>(BI->getCondition());
+  DEBUG_WITH_TYPE(VerboseDebug, if (OuterLoopLatchCmp) {
+    dbgs() << "Outer loop latch compare instruction: " << *OuterLoopLatchCmp
----------------
[unrelated to this patch] Why isn't this prefixed with `LLVM`?

[suggestion] We could `#define LLVM_DEBUG_VERBOSE(...) DEBUG_WITH_TYPE(DEBUG_TYPE "-verbose", __VA_ARGS__)`


================
Comment at: llvm/lib/Analysis/LoopNestAnalysis.cpp:92-94
+      bool isAllowed = isa<BinaryOperator>(I) || isa<CmpInst>(I) ||
+                       isa<PHINode>(I) || isa<BranchInst>(I) ||
+                       isa<CastInst>(I);
----------------
[serious] Should this just use `llvm::isSafeToSpeculativelyExecute`? It basically contains anything that could just be moved into the innermost loop such that it is executed multiple times.


================
Comment at: llvm/lib/Analysis/LoopNestAnalysis.cpp:177-180
+  LLVM_DEBUG(dbgs() << "Checking whether loop nest rooted by loop '"
+                    << Root.getName() << "' is a perfect nest\n";);
+
+  return getMaxPerfectDepth(Root, SE) == getNestDepth(Root);
----------------
[serious] This looks inconsistent with `getMaxPerfectDepth()` which understand "perfectly nested" as a property of a set of loops. That is, in
```
  for (int i)
    for (int j) {
      preStmt();
      for (int k) ...
      postStmt();
    }
```
I'd understand the loop `i` and `j` to be a perfect loop nest. What code is in the body does not matter.

It doesn't seem to be used, so I'd just remove this function.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68789