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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D105338#2864865 <https://reviews.llvm.org/D105338#2864865>, @spatel wrote:

> In D105338#2863990 <https://reviews.llvm.org/D105338#2863990>, @chandlerc wrote:
>
>> But just reverting this is pretty hostile unless Clang has been updated to make C and C++ volatile stores continue to behave as the language expects first.
>>
>> Just regressing Clang because no one has written the LangRef change doesn't make sense to me. I think you should fix Clang to not be buggy first if you want to pursue this (or work with the Clang devs to get such a patch), just like we would fix a buggy LLVM pass that broke real users before enabling a valid optimization that uncovered the big.
>
> I agree. I don't know exactly what fixes are needed for Clang [...]

Maybe it's just me but I have not understood what needs fixing in Clang. The way I understood this, the "problem" is that people think (e.g., maybe based on the blog post from 2011) that volatile store can be used to force a trap.
As shown very nicely in https://reviews.llvm.org/D87149#2266434, the argument is mood. As long as UB is hoisted across those volatile stores, they are effectively removed.

> If the goal is to get source like that to change, then we might as well fix that first, so we can point to it as a reference example of how to fix code?

We pointed them to a fix (`__builtin_trap()`) a while ago, didn't we?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105338