[PATCH] D43245: [LoopInterchange] Support reductions across inner and outer loop.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks Eli!



================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:829
-  // Outer loop cannot have reduction because then loops will not be tightly
-  // nested.
-  if (!Reductions.empty()) {
----------------
efriedma wrote:
> Given you're deleting this, is there some check that disallows interchanging if a PHI is a reduction in the outer loop, but not the inner loop?  Something like:
> 
> ```
>   for( int i=1;i<N;i++) {
>     for( int j=1;j<N;j++) {
>       A[i][j]++;
>     }
>     X += j;
>   }
> ```
Right!  We should keep the check and could just not add PHIs across inner and outer loop there I think


================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:1521
+      if (InnerLoop->contains(InBB) ||
+          isa<PHINode>(PHI->getIncomingValueForBlock(InBB)))
         continue;
----------------
efriedma wrote:
> Checking `isa<PHINode>()` here seems sort of confusing... is this supposed to be checking for a PHI node from the outer loop?  If so, is there some guarantee it isn't a PHI node from somewhere else?
Yeah, it seems like inner loop reductions are not handled properly to start with. I will look into that first. It might be helpful to keep track of reduction/induction phis after the initial checks and use that info here (and other places, rather than recomputing)


https://reviews.llvm.org/D43245