[PATCH] D87149: [InstCombine] erase instructions leading up to unreachable

[stephan.yichao.zhao via Phabricator via llvm-commits]

stephan.yichao.zhao added a comment.

> The requirements on volatile operations have been clarified in D53184 <https://reviews.llvm.org/D53184> after a llvm-dev discussion. Of course we can re-evaluate this decision, in which case a LangRef patch needs to be proposed and a new RFC on llvm-dev started.

In D53184 <https://reviews.llvm.org/D53184>, about trap, the reasoning is "If we don't restrict the effects of a volatile load/store, we have to assume it could, at worst, modify any memory that isn't local to the function and unwind the stack, like a call to an unknown function. That would block optimizations around volatile accesses. That's not what LLVM implements, and it's not what other compilers implement."

http://lists.llvm.org/pipermail/llvm-dev/2018-January/120218.html talked about a hoisting issue. The question is if to allow moving a non-volatile operation across a volatile access, do we need all restrictions: memory non-aliasing, non-looping, non-existing. These features are like a negation of a function with conservative assumptions. The first two are definitely necessary to swap a volatile access and a non-volatile access. But non-existing is not necessary. For example,

  volatile-access;
  ordinary-access;

If ordinary-access is defined, no matter volatile-access returns or exists, it is fine to swap them because there are no other external events to distinguish them. 
If ordinary-access is undefined, it is okay to swap because undefined behavior is retroactive.

So if we rewrite "The compiler may assume execution will continue after a volatile operation, so operations which modify memory or may have undefined behavior can be hoisted past a volatile operation." in LangRef into "The compiler may assume execution will continue after a volatile operation or a volatile operation can exist", hoisting still works.

Did other optimizations need all restrictions to work? https://reviews.llvm.org/D65375 removed checkings of volatile from isGuaranteedToTransferExecutionToSuccessor. After then, maybe some optimizations were turned on like this diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87149