[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:
> 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.


https://reviews.llvm.org/D53744