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

[Frank Laub via Phabricator via llvm-commits]

flaub marked an inline comment as done.
flaub 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>;
----------------
jfb wrote:
> 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.
I see, I took the namings from: https://llvm.org/docs/LangRef.html#ordering

But if that's out of date, I can update it.

Ideally, this whole dialect would be mechanically generated from LLVM IR sources/td, so whichever name matches the existing IR is probably what we should use.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72741