Implement callee/caller type checking for llvm.call

[mehdi_amini]

Summary

This aligns the behavior with the standard call as well as the LLVM verifier.

[mehdi_amini]

mlir/test/mlir-cpu-runner/bare_ptr_call_conv.mlir

143 ↗

(On Diff #294507)

@dcaballe : this is the problematic call right now.

[mehdi_amini]

Replace some auto with explicit types

[dcaballe]

Thanks, LGTM! Please, give some time for @ftynse to have a look.

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

541

it an -> it is an?

547

remove {}?

mlir/test/mlir-cpu-runner/bare_ptr_call_conv.mlir

143 ↗

(On Diff #294507)

Thanks! I should be able to enable this after https://reviews.llvm.org/D87724.

[dcaballe]

mlir/test/mlir-cpu-runner/bare_ptr_call_conv.mlir

143 ↗

(On Diff #294507)

D87724 landed already. If you uncomment the scf/standard dialect main function above and remove the llvm one, this seems to work now. Could you please rebase your patch and give it a try?

[mehdi_amini]

mlir/test/mlir-cpu-runner/bare_ptr_call_conv.mlir

143 ↗

(On Diff #294507)

Yes it works, but that seems also like orthogonal to the current patch, why didn't you do it as part of D87724?

[mehdi_amini]

mlir/test/mlir-cpu-runner/bare_ptr_call_conv.mlir

143 ↗

(On Diff #294507)

Pushed in https://github.com/llvm/llvm-project/commit/eff9984dca03 ; will rebase this patch.

[mehdi_amini]

Rebase, test is passing so removing XFAIL

[ftynse]

Could we have tests for user-visible error messages?

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

542

Using textual attribute names makes it feel like JSON. We can just have op.callee() or op.calleeAttr() if the attribute object is necessary.

Also, this seems to accept nested symbol references without verifying them.

[dcaballe]

mlir/test/mlir-cpu-runner/bare_ptr_call_conv.mlir

143 ↗

(On Diff #294507)

Yes it works, but that seems also like orthogonal to the current patch, why didn't you do it as part of D87724?

Sorry, not intentional. I thought it could be some interaction with your patch. Thanks for fixing it.

[mehdi_amini]

Add tests and add verifier for indirect calls

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

542

Copied it out the textual part from the Standard dialect verifier ;)

(the nested symbol should be covered by the native verifier from the ODS definition specifying a FlatSymbolRefAttr)

[ftynse]

Thanks!

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

542

Ah, we need to port the Standard dialect to (modern) ODS :)

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

let builders = [OpBuilder<
     [{
       LLVMType resultType = func.getType().getFunctionResultType();
       if (!resultType.isVoidTy())
         $_state.addTypes(resultType);
       $_state.addAttribute("callee", $_builder.getSymbolRefAttr(func));
       $_state.addAttributes(attributes);
       $_state.addOperands(operands);
     }]>];
-  let verifier = [{
-    if (getNumResults() > 1)
-      return emitOpError("must have 0 or 1 result");
-    return success();
-  }];
+  let verifier = [{ return ::verify(*this); }];
   let parser = [{ return parseCallOp(parser, result); }];
   let printer = [{ printCallOp(p, *this); }];
 }
 def LLVM_ExtractElementOp : LLVM_OneResultOp<"extractelement", [NoSideEffect]>,
                           Arguments<(ins LLVM_AnyVector:$vector,
                                          LLVM_AnyInteger:$position)> {
   string llvmBuilder = [{
     $res = builder.CreateExtractElement($vector, $position);

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

static ParseResult parseLandingpadOp(OpAsmParser &parser,
   if (parser.parseColon() || parser.parseType(type))
     return failure();
 
   result.addTypes(type);
   return success();
 }
 
 //===----------------------------------------------------------------------===//
-// Printing/parsing for LLVM::CallOp.
+// Verifying/Printing/parsing for LLVM::CallOp.
 //===----------------------------------------------------------------------===//
 
+static LogicalResult verify(CallOp &op) {
+  if (op.getNumResults() > 1)
+    return op.emitOpError("must have 0 or 1 result");
+
+  // Type for the callee, we'll get it differently depending if it is a direct
+  // or indirect call.
+  LLVMType fnType;
+
+  bool isIndirect = false;
+
+  // If this is an indirect call, the callee attribute is missing.
+  Optional<StringRef> calleeName = op.callee();
+  if (!calleeName) {
+    isIndirect = true;
+    if (!op.getNumOperands())
+      return op.emitOpError(
+          "must have either a `callee` attribute or at least an operand");
+    fnType = op.getOperand(0).getType().dyn_cast<LLVMType>();
+    if (!fnType)
+      return op.emitOpError("indirect call to a non-llvm type: ")
+             << op.getOperand(0).getType();
+    auto ptrType = fnType.dyn_cast<LLVMPointerType>();
+    if (!ptrType)
+      return op.emitOpError("indirect call expects a pointer as callee: ")
+             << fnType;
+    fnType = ptrType.getElementType();
+  } else {
+    Operation *callee = SymbolTable::lookupNearestSymbolFrom(op, *calleeName);
+    if (!callee)
+      return op.emitOpError()
+             << "'" << *calleeName
+             << "' does not reference a symbol in the current scope";
+    auto fn = dyn_cast<LLVMFuncOp>(callee);
+    if (!fn)
+      return op.emitOpError() << "'" << *calleeName
+                              << "' does not reference a valid LLVM function";
+
+    fnType = fn.getType();
+  }
+  if (!fnType.isFunctionTy())
+    return op.emitOpError("callee does not have a functional type: ") << fnType;
+
+  // Verify that the operand and result types match the callee.
+
+  if (!fnType.isFunctionVarArg() &&
+      fnType.getFunctionNumParams() != (op.getNumOperands() - isIndirect))
+    return op.emitOpError()
+           << "incorrect number of operands ("
+           << (op.getNumOperands() - isIndirect)
+           << ") for callee (expecting: " << fnType.getFunctionNumParams()
+           << ")";
+
+  if (fnType.getFunctionNumParams() > (op.getNumOperands() - isIndirect))
+    return op.emitOpError() << "incorrect number of operands ("
+                            << (op.getNumOperands() - isIndirect)
+                            << ") for varargs callee (expecting at least: "
+                            << fnType.getFunctionNumParams() << ")";
+
+  for (unsigned i = 0, e = fnType.getFunctionNumParams(); i != e; ++i)
+    if (op.getOperand(i + isIndirect).getType() !=
+        fnType.getFunctionParamType(i))
+      return op.emitOpError() << "operand type mismatch for operand " << i
+                              << ": " << op.getOperand(i + isIndirect).getType()
+                              << " != " << fnType.getFunctionParamType(i);
+
+  if (op.getNumResults() &&
+      op.getResult(0).getType() != fnType.getFunctionResultType())
+    return op.emitOpError()
+           << "result type mismatch: " << op.getResult(0).getType()
+           << " != " << fnType.getFunctionResultType();
+
+  return success();
+}
+
 static void printCallOp(OpAsmPrinter &p, CallOp &op) {
   auto callee = op.callee();
   bool isDirect = callee.hasValue();
 
   // Print the direct callee if present as a function attribute, or an indirect
   // callee (first operand) otherwise.
   p << op.getOperationName() << ' ';
   if (isDirect)

mlir/test/Dialect/LLVMIR/invalid.mlir

 
 func @call_non_function_type(%callee : !llvm.func<i8 (i8)>, %arg : !llvm.i8) {
   // expected-error@+1 {{expected function type}}
   llvm.call %callee(%arg) : !llvm.func<i8 (i8)>
 }
 
 // -----
 
+func @invalid_call() {
+  // expected-error@+1 {{'llvm.call' op must have either a `callee` attribute or at least an operand}}
+  "llvm.call"() : () -> ()
+}
+
+// -----
+
+func @call_non_function_type(%callee : !llvm.func<i8 (i8)>, %arg : !llvm.i8) {
+  // expected-error@+1 {{expected function type}}
+  llvm.call %callee(%arg) : !llvm.func<i8 (i8)>
+}
+
+// -----
+
+func @call_unknown_symbol() {
+  // expected-error@+1 {{'llvm.call' op 'missing_callee' does not reference a symbol in the current scope}}
+  llvm.call @missing_callee() : () -> ()
+}
+
+// -----
+
+func @standard_func_callee()
+
+func @call_non_llvm() {
+  // expected-error@+1 {{'llvm.call' op 'standard_func_callee' does not reference a valid LLVM function}}
+  llvm.call @standard_func_callee() : () -> ()
+}
+
+// -----
+
+func @call_non_llvm_indirect(%arg0 : i32) {
+  // expected-error@+1 {{'llvm.call' op operand #0 must be LLVM dialect type, but got 'i32'}}
+  "llvm.call"(%arg0) : (i32) -> ()
+}
+
+// -----
+
+llvm.func @callee_func(!llvm.i8) -> ()
+
+func @callee_arg_mismatch(%arg0 : !llvm.i32) {
+  // expected-error@+1 {{'llvm.call' op operand type mismatch for operand 0: '!llvm.i32' != '!llvm.i8'}}
+  llvm.call @callee_func(%arg0) : (!llvm.i32) -> ()
+}
+
+// -----
+
+func @indirect_callee_arg_mismatch(%arg0 : !llvm.i32, %callee : !llvm.ptr<func<void(i8)>>) {
+  // expected-error@+1 {{'llvm.call' op operand type mismatch for operand 0: '!llvm.i32' != '!llvm.i8'}}
+  "llvm.call"(%callee, %arg0) : (!llvm.ptr<func<void(i8)>>, !llvm.i32) -> ()
+}
+
+// -----
+
+llvm.func @callee_func() -> (!llvm.i8)
+
+func @callee_return_mismatch() {
+  // expected-error@+1 {{'llvm.call' op result type mismatch: '!llvm.i32' != '!llvm.i8'}}
+  %res = llvm.call @callee_func() : () -> (!llvm.i32)
+}
+
+// -----
+
+func @indirect_callee_return_mismatch(%callee : !llvm.ptr<func<i8()>>) {
+  // expected-error@+1 {{'llvm.call' op result type mismatch: '!llvm.i32' != '!llvm.i8'}}
+  "llvm.call"(%callee) : (!llvm.ptr<func<i8()>>) -> (!llvm.i32)
+}
+
+// -----
+
 func @call_too_many_results(%callee : () -> (i32,i32)) {
   // expected-error@+1 {{expected function with 0 or 1 result}}
   llvm.call %callee() : () -> (i32, i32)
 }
 
 // -----
 
 func @call_non_llvm_result(%callee : () -> (i32)) {