[PATCH] D105338: [InstCombine] Revert "Temporarily do not drop volatile stores before unreachable"

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

In D105338#2871013 <https://reviews.llvm.org/D105338#2871013>, @nlopes wrote:

> @efriedma your proposal (5) is to change the semantics of `__builtin_unreachable()` so it lowers to e.g. `@llvm.trap` rather than unreachable. I don't think having 2 `__builtin_*` with the same semantics is any useful. Plus gcc documentation clearly says that `__builtin_unreachable` triggers UB.

Sorry I wasn't quite clear.  (5) is just declaring "whatever transforms compilers currently implement are legal, and whatever transforms they don't are illegal".  I don't think there's any way to turn it into a semantically consistent model.

> Defining volatile ops as producing side-effects alone is not sufficient to prevent them from being deleted if followed by unreachable. We also delete function calls that precede unreachable.

We only delete calls before unreachable if they're `mustreturn`.  The idea behind (2) is that we can mess with whether volatile ops are considered `mustreturn` without allowing other side-effects.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105338/new/

https://reviews.llvm.org/D105338