[PATCH] D50558: [MustExecute] Fix algorithmic bug in isGuaranteedToExecute. PR38514

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

In https://reviews.llvm.org/D50558#1203595, @mkazantsev wrote:

> In https://reviews.llvm.org/D50558#1203581, @sanjoy wrote:
>
> > JFYI this is somewhat of a hornet's nest -- I believe cases like `test_impossible_exit_in_untaken_block` are actually correct for C/C++ because infloops without side effects are UB.
>
>
> I will add similar tests with volatile loads in header, so that it would be a clearly defined situation in C++ and the same problem showed up.


Actually this logic is accidentally correct right now because `isGuaranteedToTransferExecutionToSuccessor` thinks that volatile loads don't transfer execution to successors (which was under discurrion and probably needs removed) and MustExecute is super-conservative about such instructions in non-header blocks (see https://reviews.llvm.org/D50377). When MustThrow analysis becomes less restrictive, this problem becomes a real one and we hoist sdiv from such examples despite the fact that they are no longer "finite" according to C++ specification.

So the current behavior of this analysis is wrong, but due to limitations of MustThrow analysis it doesn't affect any real example. As I improve the mustthrow analysis, a lot of things like this show up.

I've added a couple of such tests as https://reviews.llvm.org/rL339983to make sure that they won't break even in spite of improved MustThrow analysis.


https://reviews.llvm.org/D50558