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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added a comment.

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

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


Sorry, I wasn't clear.  I meant to say that transforming

  while (true) {
    if (<unknown condition>) {
      int c = 1 / divisor;
      break;
    }
  }

to

  int c = 1 / divisor;
  while (true) {
    if (<unknown condition>) {
      break;
    }
  }

is correct for C++ (because non-side effecting infloops are UB) and that I believe your change will "regress" this optimization (since LLVM will not longer hoist the division).  Though I guess if no-one complains about it in a couple of days then we're fine?? :)

What you're saying, that it would be a bug even for C++ if LLVM hoisted the divide from

  while (true) {
    <volatile load>
    if (<unknown condition>) {
      int c = 1 / divisor;
      break;
    }
  }

is correct, but not quite what I'm trying to say.

Also, given what you said about `isGuaranteedToTransferExecutionToSuccessor`, perhaps we can close llvm.org/PR24078?


Repository:
  rL LLVM

https://reviews.llvm.org/D50558