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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev 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;
----------------
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.


https://reviews.llvm.org/D53744