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

[Frank Laub via Phabricator via llvm-commits]

flaub marked 8 inline comments as done.
flaub added a comment.

Thanks for the review :)

I'll try to follow up tomorrow and also look at writing a translation test (I wasn't aware of it, thanks for the heads up).



================
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)>,
----------------
ftynse wrote:
> Nit: the name `op` is unfortunate as it is often used as short for the current operation.
Is `bin_op` reasonable?


================
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"});
----------------
ftynse wrote:
> Dereferencing should not be necessary anymore since `mlir::Value` is now a value-type.
Oops, good catch.


================
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)
----------------
ftynse wrote:
> You can declare `op` and `ordering` as StringAttr directly, OpAsmParser does the check for you then.
Good to know, I'll do that in my next round.

I was attempting to follow other code in this file that looked like it was doing a similar thing.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:1448
+
+  result.attributes = attrs;
+  result.addTypes(funcType.getResults());
----------------
ftynse wrote:
> Nit: just write here directly instead of declaring a new vector of attributes?
I don't understand what this comment means.

Actually I was attempting to follow the other custom parser impls in this file.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:1454
+static bool isFPOperation(AtomicBinOp op) {
+  switch (op) {
+  case AtomicBinOp::fadd:
----------------
ftynse wrote:
> Nit: `return op == AtomicBinOp::fadd || op == AtomicBinOp::fsub;` looks 7 times shorter
Except it makes it much harder to maintain. This style, while longer, allows one to easily add a new case when a change in the IR arrives, and makes diffs much easier to comprehend too. 

On a minor note, this style matches the exact same function in the LLVM IR:
https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/IR/Instructions.h#L775



================
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())
----------------
ftynse wrote:
> 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.
Fair enough


================
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
----------------
ftynse wrote:
> I'd rather just pass the pointer in the function to make sure we only check the operation we care about here (atomicrmw)
Good idea


================
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:
> 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.
This was leftover from an earlier attempt to implement a `volatile` attribute to match the LLVM IR. However, this seems impossible in the llvm builder's present state. There's no way it seems to avoid C++ keyword issues. If I had access to a special `$_op` expansion then I could write a builder that calls on the op directly to access the attribute.

For now I'll drop the `volatile` in this test and we can figure out a plan on how to support C++ keyword conflicts more generally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72741