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

[Nuno Lopes via Phabricator via llvm-commits]

nlopes added a comment.

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

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. Anything that precedes UB can be deleted.
If we were to special case volatile ops, the implications are not local. We would need to consider that a called function may execute volatile ops.

CompCert's correctness theorem disallows removing operations before unreachable, but adopting that model in LLVM is a significant change. And I don't really see the point when users can just read the documentation and use the right intrinsic?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105338