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

@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

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

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

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

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

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())
         result.addTypes(resultType);
       result.addAttribute("callee", builder.getSymbolRefAttr(func));
       result.addAttributes(attributes);
       result.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");
+
+  // Check if there is a callee attribute specified or if it an indirect call.
+  auto fnAttr = op.getAttrOfType<FlatSymbolRefAttr>("callee");
+  if (!fnAttr) {
+    // FIXME: we should verify the type of the callee for indirect calls as
+    // well.
+    return success();
+  }
+
+  // Check caller/callee matching operands/result types.
+
+  auto callee = SymbolTable::lookupNearestSymbolFrom(op, fnAttr.getValue());
+  if (!callee)
+    return op.emitOpError()
+           << "'" << fnAttr.getValue()
+           << "' does not reference a symbol in the current scope";
+  auto fn = dyn_cast<LLVMFuncOp>(callee);
+  if (!fn)
+    return op.emitOpError() << "'" << fnAttr.getValue()
+                            << "' does not reference a valid LLVM function";
+
+  // Verify that the operand and result types match the callee.
+  auto fnType = fn.getType();
+
+  if (!fnType.isFunctionVarArg() &&
+      fnType.getFunctionNumParams() != op.getNumOperands())
+    return op.emitOpError()
+           << "incorrect number of operands (" << op.getNumOperands()
+           << ") for callee (expecting: " << fnType.getFunctionNumParams()
+           << ")";
+
+  if (fnType.getFunctionNumParams() > op.getNumOperands())
+    return op.emitOpError()
+           << "incorrect number of operands (" << op.getNumOperands()
+           << ") for varargs callee (expecting at least: "
+           << fnType.getFunctionNumParams() << ")";
+
+  for (unsigned i = 0, e = fnType.getFunctionNumParams(); i != e; ++i)
+    if (op.getOperand(i).getType() != fnType.getFunctionParamType(i))
+      return op.emitOpError() << "operand type mismatch for operand " << i
+                              << ": " << op.getOperand(i).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/mlir-cpu-runner/bare_ptr_call_conv.mlir

 // RUN: mlir-opt %s -convert-scf-to-std -convert-std-to-llvm='use-bare-ptr-memref-call-conv=1' | mlir-cpu-runner -shared-libs=%linalg_test_lib_dir/libmlir_c_runner_utils%shlibext -entry-point-result=void | FileCheck %s
 
+// This test is using a direct llvm.call to a function not in the standard
+// dialect which will fail the verifier.
+// XFAIL: *
+
 // Verify bare pointer memref calling convention. `simple_add1_add2_test`
 // gets two 2xf32 memrefs, adds 1.0f to the first one and 2.0f to the second
 // one. 'main' calls 'simple_add1_add2_test' with {1, 1} and {2, 2} so {2, 2}
 // and {4, 4} are the expected outputs.
 
 func @simple_add1_add2_test(%arg0: memref<2xf32>, %arg1: memref<2xf32>) {
   %c2 = constant 2 : index
   %c0 = constant 0 : index
^bb2: // pred: ^bb1
   llvm.store %3, %50 : !llvm.ptr<float>
   %51 = llvm.add %37, %2 : !llvm.i64
   llvm.br ^bb1(%51 : !llvm.i64)
 ^bb3: // pred: ^bb1
   %52 = llvm.mlir.constant(1 : index) : !llvm.i64
   %53 = llvm.mlir.constant(1 : index) : !llvm.i64
   %54 = llvm.extractvalue %20[1] : !llvm.struct<(ptr<float>, ptr<float>, i64, array<1 x i64>, array<1 x i64>)>
   %55 = llvm.extractvalue %36[1] : !llvm.struct<(ptr<float>, ptr<float>, i64, array<1 x i64>, array<1 x i64>)>
   llvm.call @simple_add1_add2_test(%54, %55) : (!llvm.ptr<float>, !llvm.ptr<float>) -> ()
   %56 = llvm.extractvalue %20[1] : !llvm.struct<(ptr<float>, ptr<float>, i64, array<1 x i64>, array<1 x i64>)>
   %57 = llvm.mlir.constant(0 : index) : !llvm.i64
   %58 = llvm.mlir.constant(1 : index) : !llvm.i64
   %59 = llvm.mul %1, %58 : !llvm.i64
   %60 = llvm.add %57, %59 : !llvm.i64
   %61 = llvm.getelementptr %56[%60] : (!llvm.ptr<float>, !llvm.i64) -> !llvm.ptr<float>
   %62 = llvm.load %61 : !llvm.ptr<float>
   llvm.call @print_f32(%62) : (!llvm.float) -> ()