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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

(still tracking towards landing this in T-12H unless delayed by further patches appearing)

In D105338#2865654 <https://reviews.llvm.org/D105338#2865654>, @jdoerfert wrote:

> In D105338#2865463 <https://reviews.llvm.org/D105338#2865463>, @spatel wrote:
>
>> In D105338#2865297 <https://reviews.llvm.org/D105338#2865297>, @jdoerfert wrote:
>>
>>> In D105338#2864865 <https://reviews.llvm.org/D105338#2864865>, @spatel wrote:
>>>
>>>> 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?
>>
>> Yep, and now it's in place with (cc @hctim ):
>> D105654 <https://reviews.llvm.org/D105654> / d458f379324967 <https://reviews.llvm.org/rGd458f379324967c3c408be06e21aad9bc92c54cb>
>> ...but there's a code comment and comment in PR47480 that says LLVM has a bug. I assume that means the LangRef has a bug, but nobody wants to change it?
>
> It depends on how you look at it. LangRef and blog post disagree, that's for sure. What is "correct" depends now. I would modify the blog post with a big note, add a clang warning, and start to delete the stores.
>
>> As for Clang, I don't know how we'd solve that problem (without changing LLVM) other than to say (loudly) that the blog post no longer applies, and we want code to update. Now we can at least point people to the GWP-asan patch as a reference, but as noted, there is very likely going to be fallout...
>
> Thinking out load here: What about a warning in clang if we store to a 0 literal?

That could be a very simple diag, and it could be useful i suppose,
but as seen in GWP-ASan case, it won't help people potentially expecting any volatile store trapping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105338