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

Wed Jan 15 01:23:45 PST 2020

ftynse requested changes to this revision.
ftynse added a comment.
This revision now requires changes to proceed.

Thanks! Most of my comments are minor.

Please also add a translation test to `test/Target/llvmir.mlir`.

================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:737
+def AtomicBinOpSub  : I64EnumAttrCase<"sub", 2>;
+def AtomicBinOpAnd  : I64EnumAttrCase<"_and", 3>;
+def AtomicBinOpNand : I64EnumAttrCase<"nand", 4>;
----------------
This is unfortunate that this is conflicting with C++ keywords. I differentiated between enum case names (capitalized and thus non-conflicting) and the printed syntax for linkage attributes. That required boilerplate switches in parsing and printing though. I'm fine with these names, but we should consider a general solution for this problem eventually.

================
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:
> 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.

================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:774
+def LLVM_AtomicRMWOp : LLVM_Op<"atomicrmw">,
+    Arguments<(ins AtomicBinOp:$op, LLVM_Type:$ptr, LLVM_Type:$val,
+                   AtomicOrdering:$ordering)>,
----------------
Nit: the name `op` is unfortunate as it is often used as short for the current operation.

================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:1377
+  p << op.getOperationName() << " ";
+  p << "\"" << stringifyAtomicBinOp(op.op()) << "\" ";
+  p << "\"" << stringifyAtomicOrdering(op.ordering()) << "\" ";
----------------
We tend to use single quotes for single characters (there's an overload on operator<< in some cases).

================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:1379
+  p << "\"" << stringifyAtomicOrdering(op.ordering()) << "\" ";
+  p << *op.ptr() << ", " << *op.val();
+  p.printOptionalAttrDict(op.getAttrs(), {"op", "ordering"});
----------------
Dereferencing should not be necessary anymore since `mlir::Value` is now a value-type.

================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:1433
+  // Replace the string attribute `ordering` with an integer attribute.
+  auto orderingStr = ordering.dyn_cast<StringAttr>();
+  if (!orderingStr)
----------------
You can declare `op` and `ordering` as StringAttr directly, OpAsmParser does the check for you then.

================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:1436
+    return parser.emitError(orderingLoc,
+                            "expected 'ordering' attribute of string type");
+
----------------
It would be unclear to the user what 'ordering' means because this word does not appear in the syntax. This code should disappear anyway if you use the typed parser function.

================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:1448
+
+  result.attributes = attrs;
+  result.addTypes(funcType.getResults());
----------------
Nit: just write here directly instead of declaring a new vector of attributes?

================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:1454
+static bool isFPOperation(AtomicBinOp op) {
+  switch (op) {
+  case AtomicBinOp::fadd:
----------------
Nit: `return op == AtomicBinOp::fadd || op == AtomicBinOp::fsub;` looks 7 times shorter

================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:1464
+static LogicalResult verify(AtomicRMWOp op) {
+  auto ptrType = op.ptr().getType().dyn_cast<LLVM::LLVMType>();
+  if (!ptrType || !ptrType.isPointerTy())
----------------
Auto-generated verifier runs before this and it already checked that all types are `LLVMType`. Just use `cast` here and drop the null checks below.

================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:1466
+  if (!ptrType || !ptrType.isPointerTy())
+    return op.emitOpError("expected LLVM IR pointer type for `ptr` operand");
+  auto valType = op.val().getType().dyn_cast<LLVM::LLVMType>();
----------------
I'd prefer identifying operands using their position. It may sound counter-intuitive, but it is never clear from the IR itself or the error message which operand relates to each name. And counting is easier and less context-switching then opening the doc to check the names, and then still counting in the IR to fix the right operand.

================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:1475-1479
+  if (isFPOperation(op.op())) {
+    if (!valType.getUnderlyingType()->isFloatingPointTy())
+      return op.emitOpError("expected LLVM IR floating point type");
+  }
+  return success();
----------------
Please add the check that arguments are power-of-two integers otherwise (https://llvm.org/docs/LangRef.html#i-atomicrmw), except for xchg that accepts both floats and integers.

================
Comment at: mlir/test/Dialect/LLVMIR/roundtrip.mlir:224-225
+func @atomics(%arg0 : !llvm.i32, %arg1 : !llvm.float) {
+  %0 = llvm.alloca %arg0 x !llvm.float : (!llvm.i32) -> !llvm<"float*">
+  %1 = llvm.getelementptr %0[%arg0, %arg0] : (!llvm<"float*">, !llvm.i32, !llvm.i32) -> !llvm<"float*">
+  // CHECK: llvm.atomicrmw "fadd" "unordered" %1, %arg1 {volatile} : (!llvm<"float*">, !llvm.float) -> !llvm.float
----------------
I'd rather just pass the pointer in the function to make sure we only check the operation we care about here (atomicrmw)

================
Comment at: mlir/test/Dialect/LLVMIR/roundtrip.mlir:226
+  %1 = llvm.getelementptr %0[%arg0, %arg0] : (!llvm<"float*">, !llvm.i32, !llvm.i32) -> !llvm<"float*">
+  // CHECK: llvm.atomicrmw "fadd" "unordered" %1, %arg1 {volatile} : (!llvm<"float*">, !llvm.float) -> !llvm.float
+  %2 = llvm.atomicrmw "fadd" "unordered" %1, %arg1 {volatile} : (!llvm<"float*">, !llvm.float) -> !llvm.float
----------------
The `{volatile}` part does not seem necessary for the test.

================
Comment at: mlir/test/Dialect/LLVMIR/roundtrip.mlir:226
+  %1 = llvm.getelementptr %0[%arg0, %arg0] : (!llvm<"float*">, !llvm.i32, !llvm.i32) -> !llvm<"float*">
+  // CHECK: llvm.atomicrmw "fadd" "unordered" %1, %arg1 {volatile} : (!llvm<"float*">, !llvm.float) -> !llvm.float
+  %2 = llvm.atomicrmw "fadd" "unordered" %1, %arg1 {volatile} : (!llvm<"float*">, !llvm.float) -> !llvm.float
----------------
ftynse wrote:
> The `{volatile}` part does not seem necessary for the test.
Please avoid using SSA names in the CHECK strings, there are no guarantees they are preserved. Use `%{{.*}}` instead or capture the names with `%[[name-in-tablegen:.*]]`, see https://mlir.llvm.org/getting_started/TestingGuide/ for more detail.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72741