This is an archive of the discontinued LLVM Phabricator instance.

[mlir][llvm] Make the import of LLVM IR intrinsics extensible.
ClosedPublic

Authored by gysit on Dec 20 2022, 1:16 AM.

Details

Summary

The revision introduces the LLVMImportDialectInterface to make the
import of LLVM IR intrinsics extensible. It uses a dialect interface
that enables external projects to provide their own conversion functions
for custom intrinsics. These conversion functions can rely on the
ModuleImport class to perform support tasks such as mapping LLVM
values to MLIR values or for converting types between the two worlds.

The implementation largely mirrors the export implementation. One major
difference is the dispatch to the appropriate dialect interface, since
LLVM IR intrinsics have no direct association to an MLIR dialect. The
dialect interfaces thus have to publish the supported intrinsics to
ensure incoming conversion calls are dispatched to the right dialect
interface.

The revision implements the extensible intrinsic import discussed as
part of the "extensible llvm ir import" rfc:
https://discourse.llvm.org/t/rfc-extensible-llvm-ir-import/67256/6

Diff Detail

Event Timeline

gysit created this revision.Dec 20 2022, 1:16 AM
Herald added a project: Restricted Project. · View Herald Transcript
gysit requested review of this revision.Dec 20 2022, 1:16 AM

I skimmed over the changes and added a small NIT comment. I'm not an expert on the LLVM import, so other reviews will be required.

mlir/include/mlir/Target/LLVMIR/LLVMImportInterface.h
57

Returning an ArrayRef<unsigned> might be a good idea. Explicitly creating a new SmallVector if one only reads from it (as is done further down) seems a waste.

gysit updated this revision to Diff 484518.Dec 21 2022, 3:30 AM

Address review comment.

gysit marked an inline comment as done.Dec 21 2022, 3:31 AM
gysit updated this revision to Diff 484832.Dec 22 2022, 7:18 AM

Better error message and translation naming.

Dinistro added inline comments.Dec 22 2022, 11:31 PM
mlir/include/mlir/Target/LLVMIR/LLVMImportInterface.h
72

NIT: I find this name a bit misleading as the main purpose of this function seems to be the initialization to the supported intrinsics. Consider renaming the function into something like initializeSupportedIntrinsics.

mlir/include/mlir/Target/LLVMIR/ModuleImport.h
49

NIT: As above, I would expect a query function to return said list but this function initializes it.

gysit updated this revision to Diff 485065.Dec 23 2022, 1:08 AM
gysit marked 2 inline comments as done.

Address review comments.

ftynse accepted this revision.Dec 28 2022, 4:56 AM

I think we need a bit of testing here, like checking that tblgen'ed code is what we expect it to be.

mlir/include/mlir/Target/LLVMIR/Dialect/LLVMIR/LLVMIRToLLVMImport.h
1 ↗(On Diff #485065)

Nit: could we call this LLVMIRToLLVMTranslation for symmetry with the adjacent file?

mlir/include/mlir/Target/LLVMIR/Import.h
33–37

Typo: the the

mlir/include/mlir/Target/LLVMIR/LLVMImportInterface.h
57

Please document what the result refers to here. It's unclear how an arrayref of integers maps to intrinsics.

76–85

Nit: consider swapping the condition + using continue to decrease the number of highly-indented code.

mlir/include/mlir/Tools/mlir-translate/Translation.h
111

Nit: consider a typedef instead of spelling out the function object type every time, similarly yo TranslateSourceMgrToMLIRFunction above.

mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
46

Please comment why you need to register DLTI here.

This revision is now accepted and ready to land.Dec 28 2022, 4:56 AM
gysit updated this revision to Diff 485757.Dec 31 2022, 2:13 AM
gysit marked 6 inline comments as done.

Address review comments and ensure all dialects and extensions are loaded.

Thanks for the review!