[PATCH] D53744: [SimpleLoopUnswitch] Unswitch by experimental.guard intrinsics

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2253-2255
+  // TODO: We don't know for sure that this guard will then be unswitched. We
+  // can make this statistics more accurate.
+  ++NumGuards;
----------------
mkazantsev wrote:
> chandlerc wrote:
> > mkazantsev wrote:
> > > mkazantsev wrote:
> > > > chandlerc wrote:
> > > > > What happens if the guard isn't unswitched? Will anything clean up the branch?
> > > > Nope. As far as I'm aware, `unswitchNontrivialInvariants` can only return `false` if one of successors starts with `CleanupPadInst`, which is not the case we have after this split. I believe that in practice unswitching should be always successful after this transform. I placed a `TODO` to follow-up on this in the future (I think we need to assert that fact).
> > > The only way how unswitch may fail is marked as FIXME:
> > > 
> > >   // We cannot unswitch if exit blocks contain a cleanuppad instruction as we
> > >   // don't know how to split those exit blocks.
> > >   // FIXME: We should teach SplitBlock to handle this and remove this
> > >   // restriction.
> > >   for (auto *ExitBB : ExitBlocks)
> > >     if (isa<CleanupPadInst>(ExitBB->getFirstNonPHI()))
> > >       return false;
> > > 
> > > I think it's more or less OK to not heal the guard if unswitching failed, provided that there is a plan to either make it unfailable or migrate to new guards representation in D51207.
> > I think you should just go ahead and assert this.
> > 
> > Because this code is *creating* the branch, it can (and does) ensure that the resulting branch does not go to a block with a cleanup pad. The invariant that this is an unswitchable condition should always hold and we should just verify it.
> > 
> > Then we don't even need to discuss cleanups, etc.
> Unfortunately, unswitching breaks is *any* exit block has a cleanup. :( I was able to construct such test:
> 
>   define void @test_cleanuppad(i1 %cond, i32 %N) uwtable ssp personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
> 
>   entry:
>     br label %loop
> 
>   loop:
>     %iv = phi i32 [ 0, %entry ], [ %iv.next, %loop ]
>     call void (i1, ...) @llvm.experimental.guard(i1 %cond) [ "deopt"() ]
>     %iv.next = add i32 %iv, 1
>     invoke void @may_throw(i32 %iv) to label %loop unwind label %exit
> 
>   exit:
>     %cp = cleanuppad within none []
>     cleanupret from %cp unwind to caller
> 
>   }
> 
> I think we can make the cleanup check before we make any transform, and therefore we guarantee that the unswitching is always successful.
Ah, nice test case, and yeah I like the idea of making this layer check the conditions necessary.


https://reviews.llvm.org/D53744