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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

[serious] Could you add a test with early exits, e.g.

  for (i = 0; i<ni; ++i) {
      for (j=0; j<nj; j+=1) {
         if (c)
          return i+j;
      }
  }



================
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);
----------------
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.


================
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)
----------------
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);
  }
}
```


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68789