[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:
> > 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`
I don't think you chose the right way of doing so. You only changed the way an operation is printed, without fundamentally changing anything about the operation itself. In the proposed version, there are two states of the op (no-attribute, and empty-attribute), which seem equivalent in LLVM IR. Instead, I would suggest making the attribute mandatory, and changing the parser and the builder to create the attribute and assign an empty string value to it if it is not present or provided in the builder.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75645