[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/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:
> > > 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.
The `dyn_cast` comment is still not addressed.

Normally, you don't need to go `op.getAttr().cast().getValue()`, you have an auto-generated accessor `.syncscope` for that.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:1568
+static LogicalResult verify(FenceOp &op) {
+  if ((int)op.ordering() < 4)
+    return op.emitOpError("can be given only acquire, release, acq_rel, "
----------------
Still not there. Please use named enum values instead of magic numbers. Previous version was more readable, but used expensive string comparisons for no reason. You need something like `op.ordering() != AtomicOrdering::not_atomic` or `op.ordering < AtomicOrderig::acquire`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75645