[PATCH] D37463: Fix miscompile in LoopSink pass

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In https://reviews.llvm.org/D37463#864419, @dberlin wrote:

> In https://reviews.llvm.org/D37463#861838, @mkazantsev wrote:
>
> > I need to read what C++ specification says about this particular issue, but basically LLVM is not only used to compile C++. This situation can be illegal in other languages (again, need to dig more through specifications). My proposal is to add an option that prohibits this transform and set it to `false` by default, with abitily to turn it off for languages where it is prohibited.
>
>
> LLVM has a memory model.
>  https://llvm.org/docs/LangRef.html#memory-model-for-concurrent-operations
>
> If this violates LLVM's memory model, it's a bug.
>  It it doesn't, it should not be turned off, and an other-language frontend needs to make sure the code it generates is correct in LLVM's memory model.
>  (or of course, start a discussion about changing the memory model)


Strongly agreed with this.  Neither the C++ memory model or the Java memory model are directly relevant to this discussion.  We should focus on the LLVM memory model exclusively when defining LLVM bugs and revise the LLVM memory model if needed to support one of our frontend languages.

Reading through the discussion, it sounds like we have correctly concluded there was no bug here (at least for standard non-atomic loads and stores).  Reading through the code, it also looks like ordered atomics are handled conservatively and not sunk.  I think there might be some further investigation needed for unordered atomics, but I will take that offline with Daniil before returning to this thread.


Repository:
  rL LLVM

https://reviews.llvm.org/D37463