-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[MLIR] Import LLVM add flag to disable loadalldialects #122574
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-mlir-llvm @llvm/pr-subscribers-mlir Author: William Moses (wsmoses) ChangesFull diff: https://github.com/llvm/llvm-project/pull/122574.diff 2 Files Affected:
diff --git a/mlir/include/mlir/Target/LLVMIR/Import.h b/mlir/include/mlir/Target/LLVMIR/Import.h
index 4a1242a8eb0590..4aa8f2ab7d8ce7 100644
--- a/mlir/include/mlir/Target/LLVMIR/Import.h
+++ b/mlir/include/mlir/Target/LLVMIR/Import.h
@@ -39,10 +39,13 @@ class ModuleOp;
/// be imported without elements. If set, the option avoids the recursive
/// traversal of composite type debug information, which can be expensive for
/// adversarial inputs.
+/// The `loadAllDialects` flag (default on) will load all dialects in the
+/// context.
OwningOpRef<ModuleOp>
translateLLVMIRToModule(std::unique_ptr<llvm::Module> llvmModule,
MLIRContext *context, bool emitExpensiveWarnings = true,
- bool dropDICompositeTypeElements = false);
+ bool dropDICompositeTypeElements = false,
+ bool loadAllDialects = true);
/// Translate the given LLVM data layout into an MLIR equivalent using the DLTI
/// dialect.
diff --git a/mlir/lib/Target/LLVMIR/ModuleImport.cpp b/mlir/lib/Target/LLVMIR/ModuleImport.cpp
index 2d8d7745eca9bb..f6ad1e310b1678 100644
--- a/mlir/lib/Target/LLVMIR/ModuleImport.cpp
+++ b/mlir/lib/Target/LLVMIR/ModuleImport.cpp
@@ -2325,7 +2325,8 @@ ModuleImport::translateLoopAnnotationAttr(const llvm::MDNode *node,
OwningOpRef<ModuleOp>
mlir::translateLLVMIRToModule(std::unique_ptr<llvm::Module> llvmModule,
MLIRContext *context, bool emitExpensiveWarnings,
- bool dropDICompositeTypeElements) {
+ bool dropDICompositeTypeElements,
+ bool loadAllDialects) {
// Preload all registered dialects to allow the import to iterate the
// registered LLVMImportDialectInterface implementations and query the
// supported LLVM IR constructs before starting the translation. Assumes the
@@ -2335,7 +2336,9 @@ mlir::translateLLVMIRToModule(std::unique_ptr<llvm::Module> llvmModule,
LLVMDialect::getDialectNamespace()));
assert(llvm::is_contained(context->getAvailableDialects(),
DLTIDialect::getDialectNamespace()));
- context->loadAllAvailableDialects();
+ if (loadAllDialects) {
+ context->loadAllAvailableDialects();
+ }
OwningOpRef<ModuleOp> module(ModuleOp::create(FileLineColLoc::get(
StringAttr::get(context, llvmModule->getSourceFileName()), /*line=*/0,
/*column=*/0)));
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I haven't realized we were unconditionally loading all available dialects, this is expensive for library-style usage. Any idea why we are not directly loading just the LLVM dialect and friends instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
We probably load all registered dialects to ensure the import finds all dialects that implement LLVMImportDialectInterface. I agree that we should make this optional, which is what this PR does, or in general just leave loading the dialects to the users of this function.
Co-authored-by: Oleksandr "Alex" Zinenko <[email protected]>
No description provided.