[PATCH] D47624: [PM/LoopUnswitch] Fix PR37651 by correctly invalidating SCEV when unswitching loops.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Thanks for review and for the prompting. Landing this now. Sorry I lost track of it a bit.



================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:359
+    if (OuterL)
+      SE->forgetLoop(OuterL);
+    else
----------------
mkazantsev wrote:
> chandlerc wrote:
> > mkazantsev wrote:
> > > chandlerc wrote:
> > > > mkazantsev wrote:
> > > > > Why it is enough? Imagine that you have
> > > > >   for (...) { // Loop 1
> > > > >     for (...) { // Loop 2
> > > > >       for (...) { // Loop 3
> > > > >         ...
> > > > >        }
> > > > >     }
> > > > >   }
> > > > > 
> > > > > Let `L` be Loop 3 and all exits from `L` lead to Loop 2. Then, if I understand the code correctly, OuterL will be found as Loop 2 and we invalidate it (and its internals). But what if what we are going to do with Loop 3 also affects trip count in Loop 1?
> > > > Do you know how to build such a test case? I wasn't able to craft anything, but I'm not a SCEV expert.
> > > Not sure how to construct a test that will fail an assert, however the general idea is that, for example, a block that goes to method exit from the innermost look is also an exiting block for all its containing loops, up to the topmost parent. It means that this exiting block may be considered for iter count calculation for all those loops. From this point of view, there is no practical difference between the immediate parent and any other top loop.
> > > 
> > > 
> > Yes, but this code finds the outermost exit block and uses that as the root to invalidate. So the case you're describing should be correctly handled.
> > 
> > The question is can a loop outside of this outermost exit block's loop have its iteration count change. I keep trying to build a test case for this and failing.
> > 
> > Notably, the best idea I had doesn't actually work: if you have a loop-invariant full exit from the loop nest which will make the number of loop exits go from 2 to 1 after unswitching, it might seem like this will impact every loop in the nest. But in practice, we will unswitch this one loop at a time. So if we have loops A, B, and C (C is the inner most), we will move the multi-exit from C to B in the first one. Both A and B will still be multi-exit after this, so invalidating A can't help anything. 
> I don't have a counter-argument that would convince at least myself that you are wrong, but I still don't feel 100% sure that it will be enough for all cases. However we already have experience of tracking down issues of this kind, so if there is a bug here, it is easy to catch and fix it later. Let's agree on your point.
Honestly, I'm in the same boat as you -- I don't feel 100%. But I don't think we should blindly invalidate without at least a clear rationale, and ideally a test case to demonstrate the nature of the failure.

So yeah, let's leave as-is, and if we can come up with a test case, we'll be able to expand it easily.


Repository:
  rL LLVM

https://reviews.llvm.org/D47624