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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

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

> In D105338#2867158 <https://reviews.llvm.org/D105338#2867158>, @lebedev.ri wrote:
>
>> In D105338#2867106 <https://reviews.llvm.org/D105338#2867106>, @thakis wrote:
>>
>>> To provide some data on how disruptive this is: https://bugs.chromium.org/p/chromium/issues/detail?id=1227705#c2 (Summary: A naive regex search for `\bvolatile.*(NULL|nullptr).*=` finds this pattern in at least 10 different repositories looking only at chromium code).
>>
>> That sounds fun.
>>
>>> So +1 to revert for now,
>>
>>
>>
>>> getting numbers on perf to decide if this is worth pursuing at all (they'd have to outweigh the disruption caused),
>>
>> This is irrelevant and i will not be doing that.
>> Please note that i'm *not* touching clang, i'm only changing llvm.
>> clang is not the only llvm front-end, and i'm not confident it should get to dictate
>> what temporary hacks that violate llvm langref the llvm contains.
>>
>> Tangentially, how about we propagate this hack from llvm into clang itself?
>> Since all the complaints so far are about `*(volatile int*)0 = ???` pattern specifically,
>> since we already diagnose it, we could also just codegen it as a trap directly.
>
> That seems like a good approach to me, but it should go through normal review etc :)



> Don't even need the warning change then, things will just keep working, right?

Nope. The idea is to move this hack from llvm to clang side, and then revert it on 28'th.

> (But again, please revert this for now and let's land this in some organized fashion.)
>
>>> if so then landing the clang warning, waiting a release, and only then landing the llvm change.
>>
>> as per https://llvm.org/, `July 27: release/13.x branch created`, i'd be fine with reverting for now, and relanding right after the 13.0 branching.
>> Does this work for you?
>
> Updating 10 repositories in 3 weeks sounds challenging. But maybe that's not even necessary if the frontend rewrites this to a call to __builtin_trap?

I'm only proposing to move the hack, not make it not-a-hack.
Does that sound ok to you?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105338