[PATCH] D72741: [MLIR] LLVM dialect: Add llvm.atomicrmw

Tue Jan 14 16:43:31 PST 2020

jfb added a subscriber: jyasskin.
jfb added inline comments.

================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:759
+def AtomicOrderingUnordered              : I64EnumAttrCase<"unordered", 1>;
+def AtomicOrderingMonotonic              : I64EnumAttrCase<"monotonic", 2>;
+def AtomicOrderingAcquire                : I64EnumAttrCase<"acquire", 4>;
----------------
flaub wrote:
> jfb wrote:
> > Could you call this one relaxed, so it matches C++, instead of the LLVM IR name?
> I don't have a strong intuition on this, but it seems like it should match LLVM IR faithfully since the LLVM dialect in MLIR should basically mirror it. Also, the context for this change is to support other MLIR dialect lowerings (such as the Affine dialect), and thus don't have any relationship to C++ (except perhaps distantly).
> 
> I'd suggest that we call it `relaxed` in a C++ dialect (or some other frontend), which would lower to this one.
acquire, release, ace_rel, and seq_cst came from C++ (and then C). @jyasskin might remember why monotonic wasn't called relaxed... but IMO it would be best to match the C++ naming for all these orderings, not all but one.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72741