[mlir][LLVMIR] Add support for va_start/copy/end intrinsics

[myhsu]

Summary

This patch adds three new LLVM intrinsic operations: llvm.intr.vastart/copy/end. And its translation from LLVM IR.

This effectively removes a restriction, imposed by 0126dcf1f0a1, where non-external functions in LLVM dialect cannot be variadic. At that time it was not clear how LLVM intrinsics are going to be modeled, which indirectly affects va_start/copy/end, the core intrinsics used in variadic functions. But since we have LLVM intrinsics as normal MLIR operations, it's not a problem anymore.

[ftynse]

I don't see how these connect to actual functions with variadic signatures. The change only drops the verifier, but there is no test for a variadic function with a body that uses the arguments. Ideally, we would also want an "integration" test with the mlir-cpu-runner to check that it does run + documentation in "LLVM IR Target" (speaking about which, maybe also check how this behaves with llvm.emit_c_interface).

[myhsu]

In D127540#3577151, @ftynse wrote:

I don't see how these connect to actual functions with variadic signatures. The change only drops the verifier, but there is no test for a variadic function with a body that uses the arguments.

Sorry I don't quite follow. My original thought was that the incoming LLVM IR should place the variadic-related intrinsics in the right place (we always assume input LLVM IR to be well-formed) so as a translator, we just need to make sure those intrinsics are properly translated to the corresponding operations. Plus, these intrinsics actually don't take the variadic arguments in the function signature as input (the i8 pointer argument is pointer to an allocated storage). That said, I'm happy to add a test with varadic function with body.

Ideally, we would also want an "integration" test with the mlir-cpu-runner to check that it does run + documentation in "LLVM IR Target" (speaking about which, maybe also check how this behaves with llvm.emit_c_interface).

Fair enough, I forgot to take care of the other direction (translating from LLVM dialect to LLVM IR). Will do.

[myhsu]

Add tests of variadic functions that use va_start/copy/end intrinsics. Including importer, roundtrip, convert-to-llvm, and mlir-cpu-runner tests. Note that the mlir-cpu-runner test case is X86-specific.

[myhsu]

In D127540#3577151, @ftynse wrote:

documentation in "LLVM IR Target" (speaking about which, maybe also check how this behaves with llvm.emit_c_interface).

FYI: I don't see any line in the LLVM IR Target documentation page that needs to be updated. Regarding llvm.emit_c_interface, I think you're asking about parameter packing and IIUC we only take care of such attribute on func.func rather than llvm.func, plus, I think func.func doesn't support variadic arguments either.

[ftynse]

In D127540#3593648, @myhsu wrote:

In D127540#3577151, @ftynse wrote:

documentation in "LLVM IR Target" (speaking about which, maybe also check how this behaves with llvm.emit_c_interface).

FYI: I don't see any line in the LLVM IR Target documentation page that needs to be updated. Regarding llvm.emit_c_interface, I think you're asking about parameter packing and IIUC we only take care of such attribute on func.func rather than llvm.func, plus, I think func.func doesn't support variadic arguments either.

func.func does support variadic arguments in external functions through the func.varargs attribute. This is arguably a hack. The documentation doesn't say anything because the feature has been largely unsupported. Now that it is, it makes sense to describe what exactly is supported (translation between the LLVM dialect and the LLVM IR, but not the conversion from higher levels).

One last request: please do not rename the X86Vector directory, its name is derived from the dialect of the same name. You can either prefix the new test with x86-, or create a new directory.

[myhsu]

• Move test/mlir-cpu-runner/X86/vararg.mlir to test/mlir-cpu-runner/x86-varargs.mlir and restore the X86Vector folder.
• Add documentations about converting from functions with variadic arguments.
• Disallow generating C-compatible interfaces for variadic functions.

[myhsu]

In D127540#3595838, @ftynse wrote:

In D127540#3593648, @myhsu wrote:

In D127540#3577151, @ftynse wrote:

documentation in "LLVM IR Target" (speaking about which, maybe also check how this behaves with llvm.emit_c_interface).

FYI: I don't see any line in the LLVM IR Target documentation page that needs to be updated. Regarding llvm.emit_c_interface, I think you're asking about parameter packing and IIUC we only take care of such attribute on func.func rather than llvm.func, plus, I think func.func doesn't support variadic arguments either.

func.func does support variadic arguments in external functions through the func.varargs attribute. This is arguably a hack. The documentation doesn't say anything because the feature has been largely unsupported. Now that it is, it makes sense to describe what exactly is supported (translation between the LLVM dialect and the LLVM IR, but not the conversion from higher levels).

Ideally, a dialect (that uses func.func) can support non-external variadic functions by generating llvm.intr.vastart and friends during their conversion of the function body. Thus, I don't think we should forbid variadic function conversion from higher levels.

Somewhat related: I found that it's really difficult if not impossible to generate C interfaces for variadic function (both non-external and external) because we can't forward variadic arguments easily. I also put this in TargetLLVMIR.md.

One last request: please do not rename the X86Vector directory, its name is derived from the dialect of the same name. You can either prefix the new test with x86-, or create a new directory.

mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td

 }
 
 def LLVM_CoroResumeOp : LLVM_IntrOp<"coro.resume", [], [], [], 0> {
   let arguments = (ins LLVM_i8Ptr:$handle);
   let assemblyFormat = "$handle attr-dict";
 }
 
 //
+// Variadic function intrinsics.
+//
+
+def LLVM_VaStartOp : LLVM_ZeroResultIntrOp<"vastart">,
+                     Arguments<(ins LLVM_i8Ptr:$arg_list)> {
+  let assemblyFormat = "$arg_list attr-dict";
+}
+
+def LLVM_VaCopyOp : LLVM_ZeroResultIntrOp<"vacopy">,
+                    Arguments<(ins LLVM_i8Ptr:$dest_list, LLVM_i8Ptr:$src_list)> {
+  let assemblyFormat = "$src_list `to` $dest_list attr-dict";
+}
+
+def LLVM_VaEndOp : LLVM_ZeroResultIntrOp<"vaend">,
+                   Arguments<(ins LLVM_i8Ptr:$arg_list)> {
+  let assemblyFormat = "$arg_list attr-dict";
+}
+
+//
 // Exception handling intrinsics.
 //
 
 def LLVM_EhTypeidForOp : LLVM_OneResultIntrOp<"eh.typeid.for"> {
     let arguments = (ins LLVM_i8Ptr:$type_info);
     let assemblyFormat = "$type_info attr-dict `:` type($res)";
 }
 

 def LLVM_VPFPToSIOp : LLVM_VPCastF<"fptosi">;
 
 def LLVM_VPUIToFPOp : LLVM_VPCastI<"uitofp">;
 def LLVM_VPSIToFPOp : LLVM_VPCastI<"sitofp">;
 
 def LLVM_VPPtrToIntOp : LLVM_VPCastPtr<"ptrtoint">;
 def LLVM_VPIntToPtrOp : LLVM_VPCastI<"inttoptr">;
 
-#endif // LLVM_INTRINSIC_OPS
+#endif // LLVM_INTRINSIC_OPS
 

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

if (getLinkage() != LLVM::Linkage::External &&
       return emitOpError() << "external functions must have '"
                            << stringifyLinkage(LLVM::Linkage::External)
                            << "' or '"
                            << stringifyLinkage(LLVM::Linkage::ExternWeak)
                            << "' linkage";
     return success();
   }
 
-  if (isVarArg())
-    return emitOpError("only external functions can be variadic");
-
   return success();
 }
 
 /// Verifies LLVM- and implementation-specific properties of the LLVM func Op:
 /// - entry block arguments are of LLVM types.
 LogicalResult LLVMFuncOp::verifyRegions() {
   if (isExternal())
     return success();

mlir/test/Dialect/LLVMIR/func.mlir

module {
   llvm.func weak ccc @cconv2() {
     llvm.return
   }
 
   // CHECK: llvm.func weak fastcc @cconv3
   llvm.func weak fastcc @cconv3() {
     llvm.return
   }
+
+  // CHECK-LABEL: llvm.func @variadic_def
+  llvm.func @variadic_def(...) {
+    llvm.return
+  }
 }
 
 // -----
 
 module {
   // expected-error@+1 {{requires one region}}
   "llvm.func"() {function_type = !llvm.func<void ()>, sym_name = "no_region"} : () -> ()
 }

 module {
   // expected-error@+1 {{failed to construct function type: expected zero or one function result}}
   llvm.func @foo() -> (i64, i64)
 }
 
 // -----
 
 module {
-  // expected-error@+1 {{only external functions can be variadic}}
-  llvm.func @variadic_def(...) {
-    llvm.return
-  }
-}
-
-// -----
-
-module {
   // expected-error@+1 {{cannot attach result attributes to functions with a void return}}
   llvm.func @variadic_def() -> (!llvm.void {llvm.noalias})
 }
 
 // -----
 
 module {
   // expected-error@+1 {{variadic arguments must be in the end of the argument list}}

mlir/test/Target/LLVMIR/Import/intrinsic.ll

 define void @umul_with_overflow_test(i32 %0, i32 %1, <8 x i32> %2, <8 x i32> %3) {
   ; CHECK: "llvm.intr.umul.with.overflow"(%{{.*}}, %{{.*}}) : (i32, i32) -> !llvm.struct<(i32, i1)>
   %5 = call { i32, i1 } @llvm.umul.with.overflow.i32(i32 %0, i32 %1)
   ; CHECK: "llvm.intr.umul.with.overflow"(%{{.*}}, %{{.*}}) : (vector<8xi32>, vector<8xi32>) -> !llvm.struct<(vector<8xi32>, vector<8xi1>)>
   %6 = call { <8 x i32>, <8 x i1> } @llvm.umul.with.overflow.v8i32(<8 x i32> %2, <8 x i32> %3)
   ret void
 }
 
+; CHECK-LABEL: llvm.func @va_intrinsics_test
+define void @va_intrinsics_test(i8* %0, i8* %1) {
+; CHECK: llvm.intr.vastart %{{.*}}
+  call void @llvm.va_start(i8* %0)
+; CHECK: llvm.intr.vacopy %{{.*}} to %{{.*}}
+  call void @llvm.va_copy(i8* %1, i8* %0)
+; CHECK: llvm.intr.vaend %{{.*}}
+  call void @llvm.va_end(i8* %0)
+  ret void
+}
+
 ; CHECK-LABEL:  llvm.func @coro_id
 define void @coro_id(i32 %0, i8* %1) {
   ; CHECK: llvm.intr.coro.id %{{.*}}, %{{.*}}, %{{.*}}, %{{.*}} : !llvm.token
   %3 = call token @llvm.coro.id(i32 %0, i8* %1, i8* %1, i8* null)
   ret void
 }
 
 ; CHECK-LABEL:  llvm.func @coro_begin

 declare token @llvm.coro.save(i8*)
 declare i8 @llvm.coro.suspend(token, i1)
 declare i1 @llvm.coro.end(i8*, i1)
 declare i8* @llvm.coro.free(token, i8* nocapture readonly)
 declare void @llvm.coro.resume(i8*)
 declare i32 @llvm.eh.typeid.for(i8*)
 declare i8* @llvm.stacksave()
 declare void @llvm.stackrestore(i8*)
+declare void @llvm.va_start(i8*)
+declare void @llvm.va_copy(i8*, i8*)
+declare void @llvm.va_end(i8*)
 declare <8 x i32> @llvm.vp.add.v8i32(<8 x i32>, <8 x i32>, <8 x i1>, i32)
 declare <8 x i32> @llvm.vp.sub.v8i32(<8 x i32>, <8 x i32>, <8 x i1>, i32)
 declare <8 x i32> @llvm.vp.mul.v8i32(<8 x i32>, <8 x i32>, <8 x i1>, i32)
 declare <8 x i32> @llvm.vp.sdiv.v8i32(<8 x i32>, <8 x i32>, <8 x i1>, i32)
 declare <8 x i32> @llvm.vp.udiv.v8i32(<8 x i32>, <8 x i32>, <8 x i1>, i32)
 declare <8 x i32> @llvm.vp.srem.v8i32(<8 x i32>, <8 x i32>, <8 x i1>, i32)
 declare <8 x i32> @llvm.vp.urem.v8i32(<8 x i32>, <8 x i32>, <8 x i1>, i32)
 declare <8 x i32> @llvm.vp.ashr.v8i32(<8 x i32>, <8 x i32>, <8 x i1>, i32)