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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added inline comments.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:1448
+
+  result.attributes = attrs;
+  result.addTypes(funcType.getResults());
----------------
flaub wrote:
> ftynse wrote:
> > flaub wrote:
> > > 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.
> > `parser.parseAttribute(ordering, "ordering", result.attributes)`and so on, using `result.attributes` instead of `attrs`.
> The issue is that we need to convert from a string representation to an integer one.
> 
> However, I think I've cleaned it up by parsing into a `StringAttr` and then doing the conversion followed by:
> ```
>   result.addAttribute("bin_op", binOpAttr);
> ```
> 
> 
> 
result.attributes is literally a vector of named attributes (https://github.com/llvm/llvm-project/blob/e19188af0a2690f222db7d8b866be0afef7b3da0/mlir/include/mlir/IR/OperationSupport.h#L267), fully redundant with the vector `attrs` you declared locally.

If you were able to do `attrs[0].second = <..>`, you can also do `result.attributes[0].second = <..>`


================
Comment at: mlir/test/Dialect/LLVMIR/roundtrip.mlir:223
+// CHECK-LABEL: @atomics
+func @atomics(%arg0 : !llvm<"float*">, %arg1 : !llvm.float) {
+  // CHECK: llvm.atomicrmw "fadd" "unordered" %{{.*}}, %{{.*}} : (!llvm<"float*">, !llvm.float) -> !llvm.float
----------------
Sorry for not noticing earlier, but please provide the tests for parser error messages in mlir/test/Dialect/LLVMIR/invalid.mlir as well.


================
Comment at: mlir/test/Target/llvmir.mlir:1046
+  // CHECK: atomicrmw fadd float* %{{.*}}, float %{{.*}} unordered
+  %0 = llvm.atomicrmw "fadd" "unordered" %arg0, %arg1 : (!llvm<"float*">, !llvm.float) -> !llvm.float
+  llvm.return %0 : !llvm.float
----------------
I would test each bin_op and each ordering at least once. It's mechanical but guards against subtle syntactic changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72741