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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

In https://reviews.llvm.org/D53744#1276992, @mkazantsev wrote:

> The old version of unswitching only needs a condition to unswitch on, not a particular user of this condition. That's why guards and branches can be handled in the same manner there. New one, however, also needs info about the particular user (which is currently always a terminator instruction). The boons it gives to us is that we can make a surgical DT and LI update and not invalidate these analyzes as a whole. The underlying logic in `unswitchNontrivialInvariants` deeply specializes on the terminator type (either branch of switch), and introducing a new type of a user (which is also a non-terminator) will make us write a lot of ugly duplicating code or mess up the existing code. It will also be bug prone.
>
> I think it's not worth doing because I hope that we will be able to get rid of `llvm.experimental.guard` at all once we have https://reviews.llvm.org/D51207 merged. In that case, guards will be expressed as normal branches. I put a lot of faith into this patch, that's why I don't want to mess up the existing code (or duplicate it) just to support something that isn't going to live long.


Thanks for the background context on why you're going with this approach. I think it makes total sense in that context.

I think it might be good to write some of this information down in the documentation next to the code. Something along the lines of:

  /// FIXME: Eventually, the use of this routine to handle guard intrinsics should be removed in favor of non-implicit control flow intrinsics, or re-visited to ensure we have a sustainable approach. The approach here of converting to branches is intended to be a simple and hopefully not long-term mechanism to support the existing users of guard intrinsics.


https://reviews.llvm.org/D53744