[PATCH] D34200: [PM/unswitch] Teach SimpleLoopUnswitch to do non-trivial unswitching, making it no longer even remotely simple.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc marked 4 inline comments as done.
chandlerc added inline comments.


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:1343
+      if (Loop *BBL = LI.getLoopFor(BB))
+        if (BBL == &L || !L.contains(BBL))
+          LI.changeLoopFor(BB, &ExitL);
----------------
chandlerc wrote:
> sanjoy wrote:
> > chandlerc wrote:
> > > sanjoy wrote:
> > > > I don't see when the `!L.contains(BBL)` bit will kick in -- won't all of the blocks in `ExitLoopSet` either be exit blocks of the original L, or previously interior blocks that are no longer interior?
> > > I'm not following ... exit blocks of the original L are exactly what would be identified by this check? My brain is fuzzy on this, so maybe I'm just misunderstanding the point you're making.
> > I meant to say:
> > 
> >  - `ExitLoopSet` contains `ExitBB` and a set of blocks that were internal to `L`.
> >  - The loop for `ExitBB` is already `ExitL` so we don't need to touch it.
> >  - For the blocks that were internal to `L`, `L.contains(BBL)` will never never be false except for the case when `BBL == &L`.
> Sorry, yes, this is trivially true. Sorry for not seeing this the first time around.
Err, I actually have a test case that checks this. ;] Without this, we orphan basic blocks. Added this and the check below back.


https://reviews.llvm.org/D34200