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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

My perspective:

In LLVM IR, there are basically the following possibilities for how we can handle volatile ops that trap:

1. Volatile ops can have side-effects equivalent to an external call.
2. Execution may not continue after the volatile operation (i.e. isGuaranteedToTransferExecutionToSuccessor() is false), but they can't modify arbitrary memory.  They only modify the address in question, and possibly "inaccessible" memory. (i.e. we can use AliasAnalysis::alias() to determine if a non-volatile memory operation aliases a volatile memory operation).
3. Execution will continue after the volatile operation, and they can't modify arbitrary memory.  They only modify the address in question, and possibly "inaccessible" memory.
4. They can't trap, and they can't modify memory.  They only modify "inaccessible" memory (MMIO registers, etc.).
5. (3), but don't DCE volatile ops followed by unreachable.  ("unreachable" can't be hoisted past volatile ops, but other sorts of undefined behavior, like divide by zero, can.)

(1) is terrible for performance, if you're writing embedded code that actually needs to access MMIO registers regularly.  It prevents a bunch of commonly expected transformations.  Maybe useful for supporting exotic compilation modes like clang-cl's /EHa or /volatile:ms.

(3) is basically the minimum necessary to support the C standard's rules for volatile accesses.  (2) is basically that minimum, plus volatile ops are allowed to kill the process.  I'm not sure what the impact would be overall.  Probably acceptable for most applications; isGuaranteedToTransferExecutionToSuccessor() isn't critical for the most important optimizations.

(4) is basically promising that you're only using volatile ops to access MMIO.  Maybe some programming languages would want to expose this as an intrinsic for embedded programming.

----

This patch implements (3).

As far as I can tell, all of icc, msvc, gcc, and clang currently implement (5).  See https://godbolt.org/z/Pc6Mzfvcz .  So it's basically standardized, right? :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105338