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

[Nuno Lopes via Phabricator via llvm-commits]

nlopes added a comment.

In D105338#2869953 <https://reviews.llvm.org/D105338#2869953>, @nlopes wrote:

> Let me just add a few points to the discussion:
>
> - __builtin_unreachable() in LLVM is equivalent to triggering UB.
> - Program executions that trigger UB at any point are meaningless. For the purposes of checking if an optimization is correct, we only need to consider the inputs for which no UB is triggered,
> - load/store of null is UB. And this fact is used by several analyses and optimizations in LLVM.
>
> The discussion is now around volatile load/stores of null. There are at least 3 possible semantics:
>
> 1. UB (current)
> 2. mandatory abort
> 3. non-deterministic abort
>
> The 3rd option doesn't fix the present issue, so I would discard it. Since we allow derefs to sometimes not abort execution, we could declare a trace of deref null followed by unreachable UB.
> For option 2), it adds a new requirement for CPUs that LLVM didn't previously have (that they must abort on null deref). If this a desired change, it must be clearly documented and announced in the release notes. Plus are we sure all CPUs abort on null deref in all execution modes?
>
> Let's just not try to make temporary hacks definitive or else no one will accept temporary hacks again. Which are sometimes useful and important. But they must be temporary.

Sorry, forgot to add that option 2) makes some optimizations of LLVM wrong. We can't propagate backwards that a dereferenced ptr is non-null, only forwards. So if the semantics is changed, a few analyses would need to be changed, including ValueTracking, function attribute inference, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105338