This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by myhsu on Jun 10 2022, 2:33 PM.

Details

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.

Diff Detail

Event Timeline

myhsu created this revision.Jun 10 2022, 2:33 PM
myhsu requested review of this revision.Jun 10 2022, 2:33 PM
Mogball accepted this revision.Jun 10 2022, 6:07 PM
This revision is now accepted and ready to land.Jun 10 2022, 6:07 PM
ftynse requested changes to this revision.Jun 13 2022, 2:14 AM

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

This revision now requires changes to proceed.Jun 13 2022, 2:14 AM
myhsu added a comment.Jun 14 2022, 1:22 PM

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 updated this revision to Diff 438067.Jun 17 2022, 4:55 PM

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 added a comment.Jun 17 2022, 5:15 PM

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 requested changes to this revision.Jun 20 2022, 3:04 AM

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.

This revision now requires changes to proceed.Jun 20 2022, 3:04 AM
myhsu updated this revision to Diff 438479.Jun 20 2022, 2:29 PM
  • 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.
ftynse accepted this revision.Jun 20 2022, 2:32 PM
This revision is now accepted and ready to land.Jun 20 2022, 2:32 PM
myhsu added a comment.Jun 20 2022, 2:38 PM

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.

This revision was landed with ongoing or failed builds.Jun 27 2022, 9:48 AM
This revision was automatically updated to reflect the committed changes.
mlir/test/mlir-cpu-runner/X86/vararg.mlir