[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#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.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105338