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

[Nico Weber via Phabricator via llvm-commits]

thakis added a comment.

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? (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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105338