[mlir] Added llvm.resume and personality functions in LLVM IR Dialect

[shraiysh]

Summary

llvm.resume is similar to llvm.return except that has to be exactly one operand and that should be derived from a llvm.landingpad instruction.
Any function having llvm.landingpad instruction must have a personality attribute.

Example:
LLVM IR

define dso_local i32 @main() personality i32 (...)* @__gxx_personality_v0 {
  invoke void @foo(i32 42)
          to label %3 unwind label %1

1:                                                ; preds = %0
  %2 = landingpad i8*
          catch i8** @_ZTIi
          catch i8* bitcast (i8** @_ZTIi to i8*)
  resume i8* %2

3:                                                ; preds = %0
  ret i32 1
}

MLIR - LLVM IR Dialect

llvm.func @main() -> !llvm.i32 attributes {personality = @__gxx_personality_v0} {
    %0 = llvm.mlir.constant(1 : i32) : !llvm.i32
    %1 = llvm.mlir.addressof @_ZTIi : !llvm<"i8**">
    %2 = llvm.bitcast %1 : !llvm<"i8**"> to !llvm<"i8*">
    %3 = llvm.mlir.addressof @_ZTIi : !llvm<"i8**">
    %4 = llvm.mlir.constant(42 : i32) : !llvm.i32
    llvm.invoke @foo(%4) to ^bb2 unwind ^bb1 : (!llvm.i32) -> ()
  ^bb1:	// pred: ^bb0
    %5 = llvm.landingpad (catch %3 : !llvm<"i8**">) (catch %2 : !llvm<"i8*">) : !llvm<"i8*">
    llvm.resume %5 : !llvm<"i8*">
  ^bb2:	// pred: ^bb0
    llvm.return %0 : !llvm.i32
  }

[shraiysh]

Looks like landingpad needs to be implemented before doing this. Will come back to this after that is done.

[ftynse]

Thanks for working on this! Please feel free to add me as a reviewer when you are have landingpad implemented.

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

457

The number of operands should be checked by declaring them in the "arguments" list with appropriate types. This will need another check to make sure the type matches that mentioned in some "landingpad", but that check can assume the "arguments"-based check passed.

Note that in MLIR, operation verifiers are not allowed to inspect sibling operations (since operations can be processed in parallel). So the type-match verifier should be implemented on the enclosing operation, likely in LLVMFuncOp.

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

762

This should be enforced by the verifier. Generally, we don't check the verifier-enforced properties in functions.

772

If there's strictly one operand, I'd suggest using parser.parseOperand instead. It would provide a more meaningful message in case the operand is absent.

[rriddle]

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

761

ResumeOp &

Operations are value-typed, so you shouldn't pass by reference or pointer in general.

[mehdi_amini]

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

457

Note that in MLIR, operation verifiers are not allowed to inspect sibling operations (since operations can be processed in parallel).

To clarify: this is only true for symbolic reference region-traversal: it is valid to traverse use-def chains in a verifier I believe.

[shraiysh]

Thank you all for your comments. I will get back to this issue after doing the landingpad instruction.

[shraiysh]

Please review this.

Also, I am unable to figure out how to model constant expressions for personality in MLIR.

For example,

define dso_local i32 @main() personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
  ...
}

will break right now, because I could not model bitcast globally.
Any insights into how do I get bitcast to work as a global instruction will help.
Thanks

[shraiysh]

Hi, can someone please provide a review for this patch.
Thanks.

[rriddle]

In D71888#1853128, @shraiysh wrote:

Please review this.

Also, I am unable to figure out how to model constant expressions for personality in MLIR.

For example,

define dso_local i32 @main() personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
  ...
}

will break right now, because I could not model bitcast globally.
Any insights into how do I get bitcast to work as a global instruction will help.
Thanks

This is a bit thorny. I'm not entirely sure on the contract of how the personality function is supposed to look. I went back through the commits in clang, but couldn't find anything that definitively documented it. I would say for now we could likely just always bitcast functions to an opaque i8* when exporting. That would mean that our roundtrip isn't exactly exact lossless, but that is a separate issue. Also, I have seen usages of i32 values as the personality function in some tests before. I'm not sure if any particular front-end uses anything other than a function pointer, but it is something to keep in mind. I'm fine with keeping the personality function as a FlatSymbolRef for now though.

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

487

nit: Can you override the arguments of this op so that this is handled automatically?

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

931

nit: Can you use the declarative custom assembly form instead? That would remove the need to define the parser/printer in c++.

https://mlir.llvm.org/docs/OpDefinitions/#declarative-assembly-format

Should be something like:

let assemblyFormat = "operands attr-dict `:` type(operands)";

mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp

700 ↗

(On Diff #241873)

nit: v = b.create<LandingPadOp>(...);

775 ↗

(On Diff #241873)

nit: Can you create the function and then set the personality afterwards? That seems like it would be much cleaner.

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

547 ↗

(On Diff #241873)

Can you just use the normal attribute export handling here?

612 ↗

(On Diff #241873)

What prompted this change? Seems unrelated.

mlir/test/Dialect/LLVMIR/invalid.mlir

611 ↗

(On Diff #241873)

Missing newline after this.

[rriddle]

In D71888#1863086, @shraiysh wrote:

Hi, can someone please provide a review for this patch.
Thanks.

Sorry for the delay.

[shraiysh]

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

612 ↗

(On Diff #241873)

In landingpad instruction, valuMapping was not set. Hence, while trying to make it an operand in resume, the program was going into a segmentation fault. I wrote this assert to avoid the segmentation fault.

[rriddle]

Looking good, a few more nits.

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

502

nit: Can you just use a single line string? = "...";

674

Can you fix the formatting here? Seems to be 2 spaces too many.

mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp

768 ↗

(On Diff #244323)

Can you extract this out into a new function? That should simplify a bit of the control flow here.

782 ↗

(On Diff #244323)

dyn_cast_or_null is only for when the value can be null, do we expect the operand here to ever be null? If not, use dyn_cast instead.

785 ↗

(On Diff #244323)

Please update each of the comments to have punctuation at the end. This applies throughout this revision.

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

637 ↗

(On Diff #244323)

nit: llvm::is_contained

[shraiysh]

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

637 ↗

(On Diff #244323)

I think llvm::is_contained is a wrapper for std::find. I am not sure if there is a neat way of looking for keys in a map using std::find (and hence llvm::is_contained). Please let me know if there is.

[bondhugula]

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

637 ↗

(On Diff #244323)

It might be a wrapper around it, but it's more readable to me with llvm::is_contained.

[shraiysh]

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

637 ↗

(On Diff #244323)

I meant llvm::is_contained(valueMapping, v) does not work - because valueMapping is a DenseMap. I am not aware of a variant of is_contained that supports maps - looking for a key in it. Currently I am using std::map::find while the documentation suggests is_contained is wrapper for std::find.

[mehdi_amini]

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

637 ↗

(On Diff #244323)

rather valueMapping.count(v) ?

[rriddle]

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

637 ↗

(On Diff #244323)

Gah, I was hoping that llvm::is_contained would be smart enough to detect find at this point. Given that, you should use .count() instead.

[shraiysh]

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

637 ↗

(On Diff #244323)

Done!

[shraiysh]

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

[shraiysh]

Updated with full context.

[ftynse]

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

459

Value is not guaranteed to have a defining op, use isa_and_nonnull instead to avoid a crash here.

Nit: add a space between if and (.

617

Don't we have a tablegenrated equivalent?

mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp

758 ↗

(On Diff #246057)

LLVM prefers early-return over long control flow blocks, e.g.

if (!f->hasPersinalityFn())
  return nullptr;

FlatSymbolRefAttr personality;
/* The code you have inside if, but with less indentation. */

770–771 ↗

(On Diff #246057)

LLVMDialect is cached by the Importer, so just dialect->getLLVMContext() should be sufficient

777 ↗

(On Diff #246057)

Why is this necessary? I wouldn't expect the converter to modify the input module silently.

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

98 ↗

(On Diff #246057)

Do we always want to bitcast symbol references? E.g., what happens for good old function constants that have the right type?

mlir/test/Dialect/LLVMIR/invalid.mlir

577 ↗

(On Diff #246057)

You don't need to test the default parser.

600 ↗

(On Diff #246057)

This line doesn't seem necessary in this test

[shraiysh]

Addressed comments.

mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp

777 ↗

(On Diff #246057)

If this is not done, the conversion crashes with the error Use still stuck around after Def is destroyed.

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

98 ↗

(On Diff #246057)

When the type is same, the bitcast instruction is automatically omitted in this behaviour (must be handled somewhere else). I tried running it on a function pointer for which both the types were same and it did not produce a new bitcast instruction while converting to LLVM IR.

[shraiysh]

@ftynse @rriddle please review this

[rriddle]

mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp

63 ↗

(On Diff #247171)

nit: -> /

63 ↗

(On Diff #247171)

(sigh, looks like the display is messed up)
nit: Use /// for top-level comments.

769 ↗

(On Diff #247171)

We shouldn't materialize an instruction here, you can just use ce->getOpCode() == llvm::Instruction::BitCast.

[shraiysh]

Addressed comments.

[rriddle]

mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp

780 ↗

(On Diff #249626)

Can we change these to early return instead?

if (...)

return b...;

if (auto ce ...) {

}
return FlatSymbolRefAttr();

[shraiysh]

Addressed comments.

I do not have commit access. Can someone please commit this?
Thanks

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

let verifier = [{
     if (getNumOperands() > 1)
       return emitOpError("expects at most 1 operand");
     return success();
   }];
 
   let parser = [{ return parseReturnOp(parser, result); }];
   let printer = [{ printReturnOp(p, *this); }];
 }
+def LLVM_ResumeOp : LLVM_TerminatorOp<"resume", []> {
+  let verifier = [{
+    if(getNumOperands() != 1)
+      return emitOpError("expected exactly two successors");
+    return success();
+  }];
+  let parser = [{ return parseResumeOp(parser, result); }];
+  let printer = [{ printResumeOp(p, *this); }];
+}
 def LLVM_UnreachableOp : LLVM_TerminatorOp<"unreachable", []> {
   string llvmBuilder = [{ builder.CreateUnreachable(); }];
   let parser = [{ return success(); }];
   let printer = [{ p << getOperationName(); }];
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // Auxiliary operations (do not appear in LLVM IR but necessary for the dialect
 // to work correctly).
 ////////////////////////////////////////////////////////////////////////////////
 
 // Linkage attribute is used on functions and globals. The order follows that of
 // https://llvm.org/docs/LangRef.html#linkage-types. The names are equivalent to
 // visible names in the IR rather than to enum values names in llvm::GlobalValue
 // since the latter is easier to change.
 def LinkagePrivate             : I64EnumAttrCase<"Private", 0>;
 def LinkageInternal            : I64EnumAttrCase<"Internal", 1>;
 def LinkageAvailableExternally : I64EnumAttrCase<"AvailableExternally", 2>;
 def LinkageLinkonce            : I64EnumAttrCase<"Linkonce", 3>;
 def LinkageWeak                : I64EnumAttrCase<"Weak", 4>;
 def LinkageCommon              : I64EnumAttrCase<"Common", 5>;
 def LinkageAppending           : I64EnumAttrCase<"Appending", 6>;
 def LinkageExternWeak          : I64EnumAttrCase<"ExternWeak", 7>;
 def LinkageLinkonceODR         : I64EnumAttrCase<"LinkonceODR", 8>;
 def LinkageWeakODR             : I64EnumAttrCase<"WeakODR", 9>;
 def LinkageExternal            : I64EnumAttrCase<"External", 10>;
 def Linkage : I64EnumAttr<
     "Linkage",
     "LLVM linkage types",
     [LinkagePrivate, LinkageInternal, LinkageAvailableExternally,
      LinkageLinkonce, LinkageWeak, LinkageCommon, LinkageAppending,
      LinkageExternWeak, LinkageLinkonceODR, LinkageWeakODR, LinkageExternal]> {
   let cppNamespace = "::mlir::LLVM";
 }
 
 
 def LLVM_AddressOfOp
     : LLVM_OneResultOp<"mlir.addressof">,
       Arguments<(ins FlatSymbolRefAttr:$global_name)> {
   let builders = [
     OpBuilder<"Builder *builder, OperationState &result, LLVMType resType, "
               "StringRef name, ArrayRef<NamedAttribute> attrs = {}", [{
       result.addAttribute("global_name", builder->getSymbolRefAttr(name));
       result.addAttributes(attrs);
       result.addTypes(resType);}]>,
 
     OpBuilder<"Builder *builder, OperationState &result, GlobalOp global, "
let extraClassDeclaration = [{
 
     LLVMType getType() {
       return getAttrOfType<TypeAttr>(getTypeAttrName())
           .getValue().cast<LLVMType>();
     }
     bool isVarArg() {
       return getType().getUnderlyingType()->isFunctionVarArg();
     }
 
     // Hook for OpTrait::FunctionLike, returns the number of function arguments.
     // Depends on the type attribute being correct as checked by verifyType.
     unsigned getNumFuncArguments();
 
     // Hook for OpTrait::FunctionLike, returns the number of function results.
     // Depends on the type attribute being correct as checked by verifyType.
     unsigned getNumFuncResults();
 
class LLVM_UnaryIntrinsicOp<string func, list<OpTrait> traits = []> :
     LLVM_OneResultOp<"intr." # func,
            !listconcat([NoSideEffect, SameOperandsAndResultType], traits)>,
     Arguments<(ins LLVM_Type:$in)>,
     LLVM_Builder<"$res = builder.CreateCall(llvm::Intrinsic::getDeclaration("
       "builder.GetInsertBlock()->getModule(), llvm::Intrinsic::" # func # ","
       "{$in->getType()}), {$in});"> {
 }
 
 class LLVM_BinaryIntrinsicOp<string func, list<OpTrait> traits = []> :
     LLVM_OneResultOp<"intr." # func,
            !listconcat([NoSideEffect, SameOperandsAndResultType], traits)>,
     Arguments<(ins LLVM_Type:$a, LLVM_Type:$b)>,
     LLVM_Builder<"$res = builder.CreateCall(llvm::Intrinsic::getDeclaration("
       "builder.GetInsertBlock()->getModule(), llvm::Intrinsic::" # func # ","
       "{$a->getType(), $b->getType()}), {$a, $b});"> {
 }
 

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

static ParseResult parseReturnOp(OpAsmParser &parser, OperationState &result) {
 
   if (parser.parseColonType(type) ||
       parser.resolveOperand(operands[0], type, result.operands))
     return failure();
   return success();
 }
 
 //===----------------------------------------------------------------------===//
+// Printing/parsing for LLVM::ResumeOp.
+//===----------------------------------------------------------------------===//
+
+static void printResumeOp(OpAsmPrinter &p, ResumeOp &op) {
+  assert(op.getNumOperands() == 1);
+  p << op.getOperationName();
+  p << ' ' << *op.getOperand(0) << " : " << op.getOperand(0)->getType();
+}
+
+// <operation> ::= `llvm.resume` ssa-use `:` type
+static ParseResult parseResumeOp(OpAsmParser &parser, OperationState &result) {
+  SmallVector<OpAsmParser::OperandType, 1> operands;
+  Type type;
+
+  if (parser.parseOperandList(operands) || operands.empty() ||
+      parser.parseColonType(type) ||
+      parser.resolveOperand(operands[0], type, result.operands))
+    return failure();
+
+  return success();
+}
+
+//===----------------------------------------------------------------------===//
 // Printing/parsing for LLVM::UndefOp.
 //===----------------------------------------------------------------------===//
 
 static void printUndefOp(OpAsmPrinter &p, UndefOp &op) {
   p << op.getOperationName();
   p.printOptionalAttrDict(op.getAttrs());
   p << " : " << op.res()->getType();
 }
static StringRef linkageToStr(LLVM::Linkage linkage) {
   case LLVM::Linkage::WeakODR:
     return "weak_odr";
   case LLVM::Linkage::External:
     return "external";
   }
   llvm_unreachable("unknown linkage type");
 }
 
 // Prints the keyword for the linkage type using the printer.
 static void printLinkage(OpAsmPrinter &p, LLVM::Linkage linkage) {
   p << linkageToStr(linkage);
 }
 
 static void printGlobalOp(OpAsmPrinter &p, GlobalOp op) {
   p << op.getOperationName() << ' ';
   printLinkage(p, op.linkage());
   p << ' ';

mlir/test/Dialect/LLVMIR/roundtrip.mlir

 // CHECK-LABEL: @null
 func @null() {
   // CHECK: llvm.mlir.null : !llvm<"i8*">
   %0 = llvm.mlir.null : !llvm<"i8*">
   // CHECK: llvm.mlir.null : !llvm<"{ void (i32, void ()*)*, i64 }*">
   %1 = llvm.mlir.null : !llvm<"{void(i32, void()*)*, i64}*">
   llvm.return
 }
+
+// CHECK-LABEL: @resume
+func @resume() {
+  %0 = llvm.mlir.constant(3) : !llvm.i32
+  // CHECK: llvm.resume %0 : !llvm.i32
+  llvm.resume %0 : !llvm.i32
+}
 No newline at end of file