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

[Ettore Tiotto via Phabricator via llvm-commits]

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


================
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:
> etiotto wrote:
> > 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. 
> I am arguing that we should remove `isPerfect` entirely and replace by something that returns the maximum perfect loop nest depth. To avoid a difference whether in
> ```
> for (int i)
>   for (int j) {
>     OutlinedBody(i,j);
>   }
> ```
> the function `OutlinedBody` is inlined or not.
> 
> Also, since this is a loop analysis, there should be a function determining whether the currently analyzed loop is the outermost loop nest. A loop pass could use this to skip optimizing a loop of depth 2 when when including another outer loop it could optimize a loop nest depth of 3 (since the order of optimization is determined by the LoopPassManager and can be arbitrary).
Hi Michael, the isPerfect() member function is IMO convenient for consumers that want to know whether the entire loop nest is perfect (from a syntactic standpoint). However is not strictly necessary so I'll remove it and add this member function:  

/// Return true if the given loop \p L is the outermost loop in the nest.
bool isOutermost(const Loop &L) const;


================
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);
----------------
Meinersbur wrote:
> bmahjour wrote:
> > Meinersbur wrote:
> > > [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.
> > `isSafeToSpeculativelyExecute` returns false for phi nodes, branches and most integer divisions.
> > We need to allow phi and branches due to inner loop induction and control.
> > As far as I know integer divisions are safe to speculate most of the time, except on some platforms where zero-division generate a trap. 
> > It puzzles me why `isSafeToSpeculativelyExecute` checks for integer division but completely ignores floating-point division which would have visible side-effects on more platforms!
> > 
> I guess control flow instructions are out of the scope of `isSafeToSpeculativelyExecute`, but could be checked explicitly.
> 
> `isSafeToSpeculativelyExecute` should be the reference for whether a non-control-flow instruction can be executed speculatively/redundantly. If it returns true for something that has side-effect, it's a bug, not just if called here. If it can be improved, then we can improve it. For instance, passing the target determining whether division by zero is undefined or poison (by the LangRef, it's always undefined).
> 
> Another question is whether `isSafeToSpeculativelyExecute` is what we need. It may return true on loads, but I'd exclude loads here (which may give us problems since LICM likes to move loads out if the innermost loop). If something can be moved into the innermost loop, then it does not matter whether it might be undefined behavior since execution of the innermost loop is required for anything in the innermost to execute.
> On the other side, hoisting out of the outermost loop might be required if needed to compute the iteration count of the innermost loop (e.g. for loop interchange). We cannot risk a division-by-zero to occur of it did not occur in the original loop nest.
> 
> I think `isSafeToSpeculativelyExecute` is a safe default and we may think about loosen the requirements if  we know what should happen with instructions of this kind for loop nest transformations. It's also better to have a more central place for the property we need.
In the next patch I'll will use isSafeToSpeculativelyExecute as suggested, and augment it for PHI nodes and cmp instructions.


================
Comment at: llvm/test/Analysis/LoopNestAnalysis/perfectnest.ll:224
+;   for (i = 0; i<ni; ++i)
+;     if (0<nj) { // guard branch for the j-loop
+;       for (j=0; j<nj; j+=1)
----------------
Meinersbur wrote:
> Does the guard condition be consistent with the loop exist condition? I.e. is
> ```
> for (i = 0; i<ni; ++i) {
>   if (5<nj) { // this a guard?
>     for (j=0; j<nj; j+=1) 
>       x+=(i+j);
>   }
> }
> ```
I rely on the LoopInfo getLoopGuardBranch() member function to determine whether (5<nj) is a guard for the inner loop (it is currently considered to be a guard). So the answer is yes, if (5<nj) would be considered a guard and therefore the nest considered perfect. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68789