[PATCH] D141277: [InstCombine] Don't optimize idempotent `atomicrmw <op>, 0` into `load atomic`

[Ralf via Phabricator via llvm-commits]

RalfJung added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp:135
     SI->setAlignment(DL.getABITypeAlign(RMWI.getType()));
     return eraseInstFromFunction(RMWI);
   }
----------------
RalfJung wrote:
> qcolombet wrote:
> > nikic wrote:
> > > Just to double check, is this `atomicrmw` -> `store` fold fine, or does it also have a similar issue?
> > It might, but I'd like a "language lawyer" to confirm.
> > 
> > It took me some time to get convinced that this optimization is wrong to begin with so I would rather have an actual expert on the matter to comment :).
> > 
> > @RalfJung 
> I'm afraid I am not a weak memory expert, so I don't know for sure.
> 
> But given that RMWs have special semantics that stores don't have, I am at least doubtful of this optimization. I will ask some other people. :)
> If I understand the code correctly, this will apply specifically to Xchg whose loaded value is ignored, and whose ordering is Release or Relaxed?
Judging from the discussion at <https://rust-lang.zulipchat.com/#narrow/stream/136281-t-lang.2Fwg-unsafe-code-guidelines/topic/LLVM.20atomic.20optimization.3A.20atomic.20swap.20to.20atomic.20store>, the optimization is indeed wrong since it breaks release sequences. If LLVM wants to perform such optimizations, at the very least it would have to define its own weak memory model that is very distinct from the C++ one -- and carefully justify why translating from C++ to that model is correct.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141277