[PATCH] D72741: [MLIR] LLVM dialect: Add llvm.atomicrmw
Frank Laub via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 14 16:43:31 PST 2020
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
More information about the llvm-commits
mailing list