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

[Ettore Tiotto via Phabricator via llvm-commits]

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


================
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() {
----------------
Meinersbur wrote:
> [nit]  [[ https://en.wiktionary.org/wiki/outermost | outermost ]] is a proper english word, so I suggest: `getOutermostLoop()`
ok, I will also rename getInnerMost() to getInnermost() for consistency.


================
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(); }
+
----------------
Meinersbur wrote:
> [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.
Correct. In your example loops i and j are perfectly nested with respect to each other. Loop j and k on the other hand are not perfectly nested because of the present of stmts between the loops. The entire loop nest is imperfect because of that.

So the isPerfect member function returns true if and only if any adjacent pair of loops in the nest are perfectly nested with respect to each other (not only the othermost 2 loops). That is the only loop containing user code is the innermost loop. 


================
Comment at: llvm/lib/Analysis/LoopNestAnalysis.cpp:23
+#define DEBUG_TYPE "loopnest"
+static const char *VerboseDebug = DEBUG_TYPE "-verbose";
+
----------------
Meinersbur wrote:
> Is this an established pattern? Never seen this before, but looks practical. The other pattern is `VerboseFusionDebugging`. Maybe we should agree on one patten.
I personally like this one because is shorter than  --debug-only=loopnest-verbose-debug.... but if ppl objects then I can change it.


================
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
----------------
Meinersbur wrote:
> [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__)`
Yep I like it. Would be useful to have the LLVM_DEBUG_VERBOSE macro to drive consistency in how debug traces could be toggled. 

It would be even better to have a way to pass a verbosity level to the debug/tracing infrastructure. Users would then be able to select which level of verbosity to associated with a particular debug stmt:

#define LLVM_DEBUG_VERBOSE(DbgLvl, ...) DEBUG_WITH_TYPE(DEBUG_TYPE "-verbose-" + DbgLvl, __VA_ARGS__)




================
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);
----------------
Meinersbur wrote:
> [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.
This function would return false for the`j` loop in the example because the loop nest it 'roots' is imperfect given that there is code between loops `j` and `k`. It would also return false of loop `i` given that its containing loop nest is imperfect. Does that answer your question?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68789