Skip to content

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-project 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