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

Thu Mar 5 06:35:42 PST 2020

Sagar marked an inline comment as done.
Sagar 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") << ") ";
----------------
ftynse wrote:
> 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.
I get what you mean, but I tried to replicate what opt does. 
Example:
given: `fence syncscope("") seq_cst`
opt returns: `fence seq_cst`

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75645