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

[Sagar Jain via Phabricator via llvm-commits]

Sagar marked 6 inline comments as done.
Sagar added inline comments.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:1536
+//===----------------------------------------------------------------------===//
+// Printer, parser and verifier for LLVM::FenceOp.
+//===----------------------------------------------------------------------===//
----------------
ftynse wrote:
> Can't this be implemented with declarative assembly format?
I did try that, but optional groups are only supported for variadic arguments, afaik. Essentially, I was not able to do something like, `(syncscope("$syncscope"))?`.


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


================
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") << ") ";
----------------
ftynse wrote:
> 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.
The condition was to avoid printing something like `syncscope("")`, which afaik does not hold any special meaning and so must not be printed.


================
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, "
----------------
ftynse wrote:
> I don't see the point of converting an enum to string and then comparing strings, instead of comparing enum values.
Will try to do the same with enums itself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75645