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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Thank you everyone for taking a look!

In D105338#2871623 <https://reviews.llvm.org/D105338#2871623>, @efriedma wrote:

> In D105338#2871013 <https://reviews.llvm.org/D105338#2871013>, @nlopes wrote:
>
>> @efriedma your proposal (5) is to change the semantics of `__builtin_unreachable()` so it lowers to e.g. `@llvm.trap` rather than unreachable. I don't think having 2 `__builtin_*` with the same semantics is any useful. Plus gcc documentation clearly says that `__builtin_unreachable` triggers UB.
>
> Sorry I wasn't quite clear.  (5) is just declaring "whatever transforms compilers currently implement are legal, and whatever transforms they don't are illegal".  I don't think there's any way to turn it into a semantically consistent model.
>
>> Defining volatile ops as producing side-effects alone is not sufficient to prevent them from being deleted if followed by unreachable. We also delete function calls that precede unreachable.
>
> We only delete calls before unreachable if they're `willreturn`.  The idea behind (2) is that we can mess with whether volatile ops are considered `willreturn` without allowing other side-effects.

Since volatile store isn't modelled as trapping, we could end up outlining it into a new function, and said function would obviously be `willreturn`, and thus the call to said outlined function would obviously be eraseable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105338