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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added inline comments.


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


================
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)
----------------
flaub wrote:
> 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.
This code is older than that function.  You are very welcome to modernize the rest in a separate diff :)


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:1448
+
+  result.attributes = attrs;
+  result.addTypes(funcType.getResults());
----------------
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`.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:1454
+static bool isFPOperation(AtomicBinOp op) {
+  switch (op) {
+  case AtomicBinOp::fadd:
----------------
flaub wrote:
> 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
> 
I don't expect much change happening to this part any time soon, but okay to keep as is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72741