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

[Richard Smith - zygoloid via Phabricator via llvm-commits]

rsmith added a comment.

In D105338#2867229 <https://reviews.llvm.org/D105338#2867229>, @thakis wrote:

> (I reverted this and the clang bit in 97c675d3d43fe02a0ff0a8350d79344c845758af <https://reviews.llvm.org/rG97c675d3d43fe02a0ff0a8350d79344c845758af> to "unbreak" trunk while discussions on the way forward are ongoing.)
>
> Adding @rsmith who might have an opinion on doing this in the frontend.

Example from upthread: https://github.com/llvm/llvm-project/blob/8cf60e61e7b0e3213b5b81039878dcf1ef18c00f/compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp#L254

It is not feasible to change the frontend to handle cases such as this differently, by emitting a trap directly, because the analysis required to determine whether a trap is emitted is too sophisticated.

The expectation of programmers using volatile is that you actually get a memory access, and volatile memory accesses are sometimes used in practice in cases where that memory access might have side effects on the state of the machine, meaning that execution -- in general -- does *not* necessarily continue past the instruction. Is there some way we can model that set of expectations in the LLVM IR semantics after this patch? If not, and assuming we want to keep this direction, could we add such a mechanism -- either a flag on the instruction to say that it might trap, or some kind of barrier intrinsic, or something? I think it would be fine for Clang to emit such a marker on every volatile load or store it emits; I'd even be happy to have a frontend flag to control whether we assume that volatile memory operations always complete or not, but I personally don't think it's worth adding such a thing given that volatile memory operations are extremely rare and the cost of the barrier is likely (almost) always negligible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105338