[MLIR] Added llvm.fence

[Sagar]

Summary

This patch adds llvm.fence. I tried not to change the syntax much.

syntax:

LLVM IR
fence [syncscope("<target-scope>")] <ordering>

MLIR LLVM Dialect

llvm.fence [syncscope("<target-scope>")] <ordering>

example:
LLVM IR: fence syncscope("agent") seq_cst
MLIR: llvm.fence syncscope("agent") seq_cst

[Sagar]

local was not up to date.

[Sagar]

Can anyone let me know why the build is failing? There are no errors/warnings on my local build.

[mehdi_amini]

In D75645#1907017, @Sagar wrote:

Can anyone let me know why the build is failing? There are no errors/warnings on my local build.

It tells you which test is failing. You can also click on the summary links to get more information.
In this case it seems like a clang test is broken, which is unrelated. There is also a clang-tidy fixit suggestion but it is just a warning and I'd ignore it here.

[ftynse]

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

947

Could you keep arguments in the same order as LLVM's builder (ordering, then scope).

951

Please keep C++ blobs inside Tablegen formatted as per regular C++ coding style.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

1571

Can't this be implemented with declarative assembly format?

1574

This doesn't correspond to the code below: you have a string-attr inside syncscope, not a keyword

1578

Let's have a named constant for "syncscope" instead of repeating the string every time.

1593

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.

1600

I don't see the point of converting an enum to string and then comparing strings, instead of comparing enum values.

mlir/test/Dialect/LLVMIR/roundtrip.mlir

295

Please add tests for all error messages in the custom verifier.

[Sagar]

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

1571

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"))?.

1574

Will correct that.

1593

The condition was to avoid printing something like syncscope(""), which afaik does not hold any special meaning and so must not be printed.

1600

Will try to do the same with enums itself.

[ftynse]

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

1593

"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.

[Sagar]

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

1593

I get what you mean, but I tried to replicate what opt does.
Example:
given: fence syncscope("") seq_cst
opt returns: fence seq_cst

[ftynse]

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

1593

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.

[ftynse]

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

1593

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.

1603

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.

[Sagar]

Addressed comments.

[Sagar]

typo

[Sagar]

Resolved clang-tidy warning.

[rriddle]

mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp

772

nit: Please cache the end of the range.

for (unsigned i = 0, e = ...; i != e;

[Sagar]

Addressed nits.

[Sagar]

I do not have write access to the repository. If would be great if someone could commit for me.

[ftynse]

The patch does not apply anymore, please rebase and upload a new version. Thanks!

[Sagar]

Rebased.

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

def LLVM_AssumeOp : LLVM_Op<"intr.assume", []>,
   let llvmBuilder = [{
     llvm::Module *module = builder.GetInsertBlock()->getModule();
     llvm::Function *fn =
         llvm::Intrinsic::getDeclaration(module, llvm::Intrinsic::assume, {});
     builder.CreateCall(fn, {$cond});
   }];
 }
 
+def LLVM_FenceOp : LLVM_ZeroResultOp<"fence", []>,
+                   Arguments<(ins AtomicOrdering:$ordering,
+                    StrAttr:$syncscope)> {
+  let llvmBuilder = [{
+    llvm::LLVMContext &llvmContext = builder.getContext();
+    builder.CreateFence(getLLVMAtomicOrdering($ordering),
+      llvmContext.getOrInsertSyncScopeID($syncscope));
+  }];
+  let parser = [{ return parseFenceOp(parser, result); }];
+  let printer = [{ printFenceOp(p, *this); }];
+  let verifier = "return ::verify(*this);";
+}
 #endif // LLVMIR_OPS

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

if (op.success_ordering() < AtomicOrdering::monotonic ||
     return op.emitOpError("ordering must be at least 'monotonic'");
   if (op.failure_ordering() == AtomicOrdering::release ||
       op.failure_ordering() == AtomicOrdering::acq_rel)
     return op.emitOpError("failure ordering cannot be 'release' or 'acq_rel'");
   return success();
 }
 
 //===----------------------------------------------------------------------===//
+// Printer, parser and verifier for LLVM::FenceOp.
+//===----------------------------------------------------------------------===//
+
+// <operation> ::= `llvm.fence` (`syncscope(`strAttr`)`)? keyword
+// attribute-dict?
+static ParseResult parseFenceOp(OpAsmParser &parser, OperationState &result) {
+  StringAttr sScope;
+  StringRef syncscopeKeyword = "syncscope";
+  if (!failed(parser.parseOptionalKeyword(syncscopeKeyword))) {
+    if (parser.parseLParen() ||
+        parser.parseAttribute(sScope, syncscopeKeyword, result.attributes) ||
+        parser.parseRParen())
+      return failure();
+  } else {
+    result.addAttribute(syncscopeKeyword,
+                        parser.getBuilder().getStringAttr(""));
+  }
+  if (parseAtomicOrdering(parser, result, "ordering") ||
+      parser.parseOptionalAttrDict(result.attributes))
+    return failure();
+  return success();
+}
+
+static void printFenceOp(OpAsmPrinter &p, FenceOp &op) {
+  StringRef syncscopeKeyword = "syncscope";
+  p << op.getOperationName() << ' ';
+  if (!op.getAttr(syncscopeKeyword).cast<StringAttr>().getValue().empty())
+    p << "syncscope(" << op.getAttr(syncscopeKeyword) << ") ";
+  p << stringifyAtomicOrdering(op.ordering());
+}
+
+static LogicalResult verify(FenceOp &op) {
+  if (op.ordering() == AtomicOrdering::not_atomic ||
+      op.ordering() == AtomicOrdering::unordered ||
+      op.ordering() == AtomicOrdering::monotonic)
+    return op.emitOpError("can be given only acquire, release, acq_rel, "
+                          "and seq_cst orderings");
+  return success();
+}
+
+//===----------------------------------------------------------------------===//
 // LLVMDialect initialization, type parsing, and registration.
 //===----------------------------------------------------------------------===//
 
 namespace mlir {
 namespace LLVM {
 namespace detail {
 struct LLVMDialectImpl {
   LLVMDialectImpl() : module("LLVMDialectModule", llvmContext) {}

mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp

static const DenseMap<unsigned, StringRef> opcMap = {
     // FIXME: fneg
     INST(Add, Add), INST(FAdd, FAdd), INST(Sub, Sub), INST(FSub, FSub),
     INST(Mul, Mul), INST(FMul, FMul), INST(UDiv, UDiv), INST(SDiv, SDiv),
     INST(FDiv, FDiv), INST(URem, URem), INST(SRem, SRem), INST(FRem, FRem),
     INST(Shl, Shl), INST(LShr, LShr), INST(AShr, AShr), INST(And, And),
     INST(Or, Or), INST(Xor, XOr), INST(Alloca, Alloca), INST(Load, Load),
     INST(Store, Store),
     // Getelementptr is handled specially.
-    INST(Ret, Return),
-    // FIXME: fence
+    INST(Ret, Return), INST(Fence, Fence),
     // FIXME: atomiccmpxchg
     // FIXME: atomicrmw
     INST(Trunc, Trunc), INST(ZExt, ZExt), INST(SExt, SExt),
     INST(FPToUI, FPToUI), INST(FPToSI, FPToSI), INST(UIToFP, UIToFP),
     INST(SIToFP, SIToFP), INST(FPTrunc, FPTrunc), INST(FPExt, FPExt),
     INST(PtrToInt, PtrToInt), INST(IntToPtr, IntToPtr), INST(BitCast, Bitcast),
     INST(AddrSpaceCast, AddrSpaceCast),
     // FIXME: cleanuppad
static ICmpPredicate getICmpPredicate(llvm::CmpInst::Predicate p) {
   case llvm::CmpInst::Predicate::ICMP_UGT:
     return LLVM::ICmpPredicate::ugt;
   case llvm::CmpInst::Predicate::ICMP_UGE:
     return LLVM::ICmpPredicate::uge;
   }
   llvm_unreachable("incorrect comparison predicate");
 }
 
+static AtomicOrdering getLLVMAtomicOrdering(llvm::AtomicOrdering ordering) {
+  switch (ordering) {
+  case llvm::AtomicOrdering::NotAtomic:
+    return LLVM::AtomicOrdering::not_atomic;
+  case llvm::AtomicOrdering::Unordered:
+    return LLVM::AtomicOrdering::unordered;
+  case llvm::AtomicOrdering::Monotonic:
+    return LLVM::AtomicOrdering::monotonic;
+  case llvm::AtomicOrdering::Acquire:
+    return LLVM::AtomicOrdering::acquire;
+  case llvm::AtomicOrdering::Release:
+    return LLVM::AtomicOrdering::release;
+  case llvm::AtomicOrdering::AcquireRelease:
+    return LLVM::AtomicOrdering::acq_rel;
+  case llvm::AtomicOrdering::SequentiallyConsistent:
+    return LLVM::AtomicOrdering::seq_cst;
+  }
+  llvm_unreachable("incorrect atomic ordering");
+}
+
 // `br` branches to `target`. Return the branch arguments to `br`, in the
 // same order of the PHIs in `target`.
 LogicalResult
 Importer::processBranchArgs(llvm::Instruction *br, llvm::BasicBlock *target,
                             SmallVectorImpl<Value> &blockArguments) {
   for (auto inst = target->begin(); isa<llvm::PHINode>(inst); ++inst) {
     auto *PN = cast<llvm::PHINode>(&*inst);
     Value value = processValue(PN->getIncomingValueForBlock(br->getParent()));
if (llvm::Function *callee = ii->getCalledFunction()) {
                               normalArgs, blocks[ii->getUnwindDest()],
                               unwindArgs);
     }
 
     if (!ii->getType()->isVoidTy())
       v = op->getResult(0);
     return success();
   }
+  case llvm::Instruction::Fence: {
+    StringRef syncscope;
+    SmallVector<StringRef, 4> ssNs;
+    llvm::LLVMContext &llvmContext = dialect->getLLVMContext();
+    llvm::FenceInst *fence = cast<llvm::FenceInst>(inst);
+    llvmContext.getSyncScopeNames(ssNs);
+    int fenceSyncScopeID = fence->getSyncScopeID();
+    for (unsigned i = 0, e = ssNs.size(); i != e; i++) {
+      if (fenceSyncScopeID == llvmContext.getOrInsertSyncScopeID(ssNs[i])) {
+        syncscope = ssNs[i];
+        break;
+      }
+    }
+    b.create<FenceOp>(loc, getLLVMAtomicOrdering(fence->getOrdering()),
+                      syncscope);
+    return success();
+  }
   case llvm::Instruction::GetElementPtr: {
     // FIXME: Support inbounds GEPs.
     llvm::GetElementPtrInst *gep = cast<llvm::GetElementPtrInst>(inst);
     SmallVector<Value, 4> ops;
     for (auto *op : gep->operand_values()) {
       Value value = processValue(op);
       if (!value)
         return failure();

mlir/test/Dialect/LLVMIR/invalid.mlir

llvm.func @caller(%arg0: !llvm.i32) -> !llvm.i32 {
   %1 = llvm.invoke @foo(%0) to ^bb1 unwind ^bb2 : (!llvm.i32) -> !llvm.i32
 ^bb1: // pred: ^bb0
   llvm.return %0 : !llvm.i32
 ^bb2: // pred: ^bb0
   // expected-error@+1 {{landingpad instruction expects at least one clause or cleanup attribute}}
   %2 = llvm.landingpad : !llvm<"{ i8*, i32 }">
   llvm.return %0 : !llvm.i32
 }
+
+// -----
+
+func @invalid_ordering_in_fence() {
+  // expected-error @+1 {{can be given only acquire, release, acq_rel, and seq_cst orderings}}
+  llvm.fence syncscope("agent") monotonic
+}

mlir/test/Dialect/LLVMIR/roundtrip.mlir

 func @useFreezeOp(%arg0: !llvm.i32) {
   // CHECK:  = llvm.freeze %[[ARG0:.*]] : !llvm.i32
   %0 = llvm.freeze %arg0 : !llvm.i32
   // CHECK: %[[x:.*]] = llvm.mlir.undef : !llvm.i8
   %1 = llvm.mlir.undef : !llvm.i8
   // CHECK:  = llvm.freeze %[[x]] : !llvm.i8
   %2 = llvm.freeze %1 : !llvm.i8
   return
+}
+
+// CHECK-LABEL: @useFenceInst
+func @useFenceInst() {
+  // CHECK:  syncscope("agent") seq_cst 
+  llvm.fence syncscope("agent") seq_cst
+  // CHECK:  seq_cst 
+  llvm.fence syncscope("") seq_cst
+  // CHECK:  release
+  llvm.fence release
+  return
 }
 No newline at end of file

mlir/test/Target/import.ll

 define i32 @useFreezeOp(i32 %x) {
   ;CHECK: %{{[0-9]+}} = llvm.freeze %{{[0-9a-z]+}} : !llvm.i32
   %1 = freeze i32 %x
   %2 = add i8 10, 10
   ;CHECK: %{{[0-9]+}} = llvm.freeze %{{[0-9]+}} : !llvm.i8
   %3 = freeze i8 %2
   %poison = add nsw i1 0, undef
   ret i32 0
+}
+
+;CHECK-LABEL: @useFenceInst
+define i32 @useFenceInst() {
+  ;CHECK: llvm.fence syncscope("agent") seq_cst
+  fence syncscope("agent") seq_cst
+  ;CHECK: llvm.fence release
+  fence release
+  ;CHECK: llvm.fence seq_cst
+  fence syncscope("") seq_cst
+  ret i32 0
 }
 No newline at end of file

mlir/test/Target/llvmir.mlir

 // CHECK-LABEL: @boolConstArg
 llvm.func @boolConstArg() -> !llvm.i1 {
   // CHECK: ret i1 false
   %0 = llvm.mlir.constant(true) : !llvm.i1
   %1 = llvm.mlir.constant(false) : !llvm.i1
   %2 = llvm.and %0, %1 : !llvm.i1
   llvm.return %2 : !llvm.i1
 }
+
+// CHECK-LABEL: @callFenceInst
+llvm.func @callFenceInst() {
+  // CHECK: fence syncscope("agent") release
+  llvm.fence syncscope("agent") release
+  // CHECK: fence release
+  llvm.fence release
+  // CHECK: fence release
+  llvm.fence syncscope("") release
+  llvm.return
+}
 No newline at end of file