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

[JF Bastien via Phabricator via llvm-commits]

jfb added a subscriber: reames.
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>;
----------------
mehdi_amini wrote:
> jyasskin wrote:
> > ftynse wrote:
> > > flaub wrote:
> > > > flaub wrote:
> > > > > 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.
> > > > I think we should stay in sync with: https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/Support/AtomicOrdering.h#L81
> > > I am quite strongly opposed to diverging from LLVM names here since the LLVM dialect is modeling LLVM IR, not C++. If we change the name in LLVM IR, we can do the same in the dialect. I have a vague preference for reusing enum names rather than IR keywords.
> > > 
> > > The documentation in LLVM seems in sync with the code.
> > > 
> > > > 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.
> > > 
> > > Yes, except that LLVM does not have a .td for instructions, only intrinsics. I am (lazily) thinking in the direction of having a single source of truth for instructions.
> > Since LLVM needed to support Java as well as C++, I felt that `relaxed` didn't do a good enough job of expressing the difference between their lowest levels of atomicity. I drew "monotonic" from the 4th portability assumption in the "correctness argument" in http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2325.html (though that paper doesn't actually suggest using the term as a name for anything). Java non-volatile variables provide defined behavior for racy access, but they don't provide this property, so they got the `unordered` term.
> > Could you call this one relaxed, so it matches C++, instead of the LLVM IR name?
> 
> @jfb : could we rename the LLVM IR name to align everyone on the C++ name?
> @jfb : could we rename the LLVM IR name to align everyone on the C++ name?

I think that would make sense, as long as @reames concurs. It's always seemed to be a minor mistake to me, it adds confusion when handling atomics inside of LLVM IR, and it would be a shame for a new IR to mimic it for consistency's sake.

That being said, it's by far not the most confusing thing in LLVM :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72741