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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

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.



================
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:
> 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).


https://reviews.llvm.org/D53744