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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp:135
     SI->setAlignment(DL.getABITypeAlign(RMWI.getType()));
     return eraseInstFromFunction(RMWI);
   }
----------------
qcolombet wrote:
> RalfJung wrote:
> > 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.
> @reames what do you think?
> 
> From what @RalfJung is reporting (thanks Ralf BTW), the change that replaces xchg with atomic store is technically incorrect.
> I.e., #2 from https://reviews.llvm.org/D58290.
> 
> I'm happy to remove that change in that PR too.
> Otherwise I would suggest we move with at least the current fix since it produces miscompiles in the wild.
Patch for this particular issue is at https://reviews.llvm.org/D142097


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141277