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

Mon Jul 12 09:02:14 PDT 2021

jfb added a subscriber: eli.friedman.
jfb added a comment.

I agree with @eli.friedman's breakdown of options here: https://reviews.llvm.org/D105338#2870156

That said, programmers generally assume that `volatile` effects will same-thread happens-before other things. Yes it's misplaced, yes the LangRef says otherwise. So we have a few choices:

1. Argue with developers that they're wrong. This is often a great way to get into shouting matches, and get developers to despise compiler developers. "But the LangRef was clear!!!" really isn't a compelling argument. In particular, "it's always worked before" is a strong argument, "clang can't even model your semantics" as @rsmith points out, and "OK but you haven't given me a better tool to do what I want, and I don't have a good way to find all the places you're saying my code is 'broken' now".
2. Change the LangRef to reflect developer expectations. This might cause performance degradation, for example when mixing `__builtin_unreachable` with `volatile`, or when doing really basic MMIO accesses, or when `__builtin_unreachable` now has to assume that *any* function call might contain `volatile`, so can't eliminate opaque calls that come before it.

I don't have a strong opinion on which direction we should go here. We're talking about interactions between `volatile` which is ill-defined (and defined by what programmers feel it should do), and other features such as `__builtin_unreachable`. Without good perf data, and data about at-scale breakage, it's hard to find the right solution.

I do think we should get data, and be empathetic towards our developers :)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105338