[PATCH] D75645: [MLIR] Added llvm.fence

[Alex Zinenko via Phabricator via llvm-commits]

ftynse requested changes to this revision.
ftynse added inline comments.
This revision now requires changes to proceed.


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:853
+def LLVM_FenceOp : LLVM_ZeroResultOp<"fence", []>,
+                   Arguments<(ins OptionalAttr<StrAttr>:$syncscope,
+                   AtomicOrdering:$ordering)> {
----------------
Could you keep arguments in the same order as LLVM's builder (ordering, then scope). 


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:857
+    llvm::LLVMContext &llvmContext = builder.getContext();
+    if($syncscope.hasValue())
+      builder.CreateFence(getLLVMAtomicOrdering($ordering), llvmContext.getOrInsertSyncScopeID($syncscope.getValue()));
----------------
Please keep C++ blobs inside Tablegen formatted as per regular C++ coding style.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:1536
+//===----------------------------------------------------------------------===//
+// Printer, parser and verifier for LLVM::FenceOp.
+//===----------------------------------------------------------------------===//
----------------
Can't this be implemented with declarative assembly format?


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:1539
+
+// <operation> ::= `llvm.fence` (`syncscope(`keyword`)`)?
+//                  keyword attribute-dict?
----------------
This doesn't correspond to the code below: you have a string-attr inside `syncscope`, not a keyword


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:1543
+  StringAttr sScope;
+  if (!failed(parser.parseOptionalKeyword("syncscope"))) {
+    if (parser.parseLParen() ||
----------------
Let's have a named constant for `"syncscope"` instead of repeating the string every time.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:1558
+  if (op.getAttr("syncscope") &&
+      !op.getAttr("syncscope").dyn_cast<StringAttr>().getValue().equals(""))
+    p << "syncscope(" << op.getAttr("syncscope") << ") ";
----------------
Why this condition here? Do you assign special meaning to empty scope identifier? This effectively makes a fence op with empty scope identifier and one without scope identifier print identically...

Also, `dyn_cast` may return null, use `cast` to assert instead.
Also, use `.empty()` for emptiness check.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:1565
+  StringRef ord = stringifyAtomicOrdering(op.ordering());
+  if (ord.equals("unordered") || ord.equals("monotonic"))
+    return op.emitOpError("fence can be given only acquire, release, acq_rel, "
----------------
I don't see the point of converting an enum to string and then comparing strings, instead of comparing enum values.


================
Comment at: mlir/test/Dialect/LLVMIR/roundtrip.mlir:295
   return
+}
+
----------------
Please add tests for all error messages in the custom verifier.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75645