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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added inline comments.


================
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") << ") ";
----------------
Sagar wrote:
> 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.
"Does not hold special meaning" does not mean it should not be printed in the custom form. Quite the inverse. If you print it in the generic form, it _will_ have the attribute with an empty string as value. This is _different_ from not having an attribute at all, and we should be able to roundtrip this properly, which your code does not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75645