[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#2868257 <https://reviews.llvm.org/D105338#2868257>, @lebedev.ri wrote:

> I think we should separate two patterns:
>
> 1. `*(volatile int)0 = 0`, aka let's trap
> 2. `volatile int* ptr; <...> *ptr = x;`
>
> Indeed, we can't do anything reasonable with the latter.
> Langref states that they do not trap, D53184 <https://reviews.llvm.org/D53184>, so the clang is already miscompiling the source C code that expects them to trap.

In the absence of the recent semantics change, are there actually end-to-end miscompilations, or do you instead mean that Clang is producing IR that the IR semantics don't guarantee to behave as expected (but do in practice)? Those are two quite different outcomes, especially given that there's code in our own compiler-rt (and presumably in lots of other places) that assumes that a volatile null dereference gives an actual null dereference even if the null pointer is not "immediate".

> Unless someone comes forward with an RFC to change the semantics, LLVM will continue to believe that they will not trap.

My question stands: is there a way for a frontend to generate IR that gives the semantics that Clang expects here? Or is the outcome that, after this change to the de facto semantics (and the earlier change to the de jure semantics in D53184 <https://reviews.llvm.org/D53184>), LLVM IR can no longer represent C programs with the semantics that Clang expects?

If there's nothing today, we could presumably add something like an intrinsic that is not guaranteed to return and that reads inaccessible memory only, that gets lowered to a no-op, and Clang could insert a call to that after every volatile load or store. That would seem more robust to me than an ad-hoc syntactic check for only pattern 1, given we know that pattern 2 also exists in the wild, and Clang has promised that both patterns work for many years.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105338