[PATCH] D72995: [MLIR] LLVM Dialect: add llvm.cmpxchg and improve llvm.atomicrmw custom parser

[JF Bastien via Phabricator via llvm-commits]

jfb added inline comments.


================
Comment at: mlir/test/Dialect/LLVMIR/invalid.mlir:499
+func @cmpxchg_success_must_be_stronger(%i32_ptr : !llvm<"i32*">, %i32 : !llvm.i32) {
+  // expected-error at +1 {{failure ordering must be no stronger than success ordering}}
+  %0 = llvm.cmpxchg %i32_ptr, %i32, %i32 monotonic acquire : !llvm.i32
----------------
flaub wrote:
> jfb wrote:
> > That restriction came from C++11 and is no longer in place in C++17:
> > http://wg21.link/P0418
> > See note on `failure` on https://en.cppreference.com/w/cpp/atomic/atomic/compare_exchange
> > 
> > Seems like a bug in LLVM IR to enforce this, would be good if you could fix LLVM IR and not copy that specific mistake :)
> Thanks for the heads up and references. I can attempt another patch later to bring us up to the C++17 standard. It looks like we'll need to :
>   - Update any LLVM IR validation (I haven't looked yet how LLVM IR gets validated, I'm only familiar with MLIR at the moment). 
>   - Update any documentation (I got the current set of restrictions from https://llvm.org/docs/LangRef.html#cmpxchg-instruction.
>   - Remove this restriction from the MLIR LLVM dialect
> 
I think you can move this patch forward without the restriction, and do any LLVM IR changes in parallel? I'm fine with it anyways :)

FWIW even older clang generates code for this in C++11 mode: https://godbolt.org/z/GyiC6Y


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72995