Page MenuHomePhabricator

[mlir] Use the interface-based translation for LLVM "intrinsic" dialects
ClosedPublic

Authored by ftynse on Feb 12 2021, 4:07 AM.

Details

Summary

Port the translation of five dialects that define LLVM IR intrinsics
(LLVMAVX512, LLVMArmNeon, LLVMArmSVE, NVVM, ROCDL) to the new dialect
interface-based mechanism. This allows us to remove individual translations
that were created for each of these dialects and just use one common
MLIR-to-LLVM-IR translation that potentially supports all dialects instead,
based on what is registered and including any combination of translatable
dialects. This removal was one of the main goals of the refactoring.

To support the addition of GPU-related metadata, the translation interface is
extended with the amendOperation function that allows the interface
implementation to post-process any translated operation with dialect attributes
from the dialect for which the interface is implemented regardless of the
operation's dialect. This is currently applied to "kernel" functions, but can
be used to construct other metadata in dialect-specific ways without
necessarily affecting operations.

Depends On D96591, D96504

Diff Detail

Event Timeline

ftynse created this revision.Feb 12 2021, 4:07 AM
ftynse requested review of this revision.Feb 12 2021, 4:07 AM
aartbik added inline comments.Feb 12 2021, 5:40 PM
mlir/include/mlir/Target/LLVMIR/Dialect/LLVMAVX512/LLVMAVX512ToLLVMIRTranslation.h
21

typo: belonging

mlir/include/mlir/Target/LLVMIR/Dialect/LLVMArmNeon/LLVMArmNeonToLLVMIRTranslation.h
21

ditto

mlir/include/mlir/Target/LLVMIR/Dialect/LLVMArmSVE/LLVMArmSVEToLLVMIRTranslation.h
21

and one more, and below, won't repeat

ftynse updated this revision to Diff 323703.Feb 15 2021, 2:51 AM
ftynse marked 3 inline comments as done.

Rebase and configure build

nicolasvasilache accepted this revision.Feb 15 2021, 4:04 AM
nicolasvasilache added inline comments.
mlir/lib/Target/LLVMIR/ConvertToLLVMIR.cpp
83–85

Do we intend to pull the kitchen sink in the longer term or is it just transient state waiting for e.g. better opt-in CMake support?

This revision is now accepted and ready to land.Feb 15 2021, 4:04 AM
ftynse added inline comments.Feb 15 2021, 4:18 AM
mlir/lib/Target/LLVMIR/ConvertToLLVMIR.cpp
83–85

I'll refactor it a bit further to have a mlir::registerAllLLVMIRTranslations by analogy with mlir::registerAllDialects used in mlir-opt, but we will have to have explicit registration somewhere unless we want link-time registration, which would contradict the rest of MLIR.

This revision was landed with ongoing or failed builds.Feb 15 2021, 5:43 AM
This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Feb 15 2021, 9:19 AM
mlir/lib/Target/LLVMIR/Dialect/NVVM/NVVMToLLVMIRTranslation.cpp
53

Should this be in a verifier?

mlir/test/Target/avx512.mlir
1

Was this totally unnecessary ? We have coverage for the round-trip of whatever is here?

ftynse added inline comments.Feb 16 2021, 9:08 AM
mlir/lib/Target/LLVMIR/Dialect/NVVM/NVVMToLLVMIRTranslation.cpp
53
mlir/test/Target/avx512.mlir
1

There is no custom syntax for these ops, and there are no diagnostics checks either. Otherwise, I would have split out the roundtripping part anyway, "target" tests should not test it.

ftynse marked 2 inline comments as done.Feb 16 2021, 9:08 AM