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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D105338#2865215 <https://reviews.llvm.org/D105338#2865215>, @chandlerc wrote:

> 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, but we do know that code like this:
>> https://github.com/llvm/llvm-project/blob/8cf60e61e7b0e3213b5b81039878dcf1ef18c00f/compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp#L254
>> ( the root cause of https://llvm.org/PR47480 ) is going to break again with this change.
>>
>> 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?
>
> I really don't know how to change that code in a meaningful way. It is already marking the store as volatile (in the C sense).

The suggestion there was to change `__builtin_unreachable()` to `__builtin_trap()`. Is that not an adequate fix?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105338