Skip to content

[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

Merged
merged 2 commits into from
Jan 11, 2025
Merged

Conversation

wsmoses
Copy link
Member

@wsmoses wsmoses commented Jan 11, 2025

No description provided.

@llvmbot
Copy link
Member

llvmbot commented Jan 11, 2025

@llvm/pr-subscribers-mlir-llvm

@llvm/pr-subscribers-mlir

Author: William Moses (wsmoses)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/122574.diff

2 Files Affected:

  • (modified) mlir/include/mlir/Target/LLVMIR/Import.h (+4-1)
  • (modified) mlir/lib/Target/LLVMIR/ModuleImport.cpp (+5-2)
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)));

Copy link
Member

@ftynse ftynse left a 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?

Copy link
Contributor

@gysit gysit left a 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.

@wsmoses wsmoses merged commit 38fcf62 into llvm:main Jan 11, 2025
8 checks passed
BaiXilin pushed a commit to BaiXilin/llvm-fix-vnni-instr-types that referenced this pull request Jan 12, 2025
@wsmoses wsmoses deleted the loadall branch January 13, 2025 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants