[MLIR][LLVM] Turn the inliner interface into a promised interface #103927

Dinistro · 2024-08-14T13:15:05Z

This commit changes the LLVM dialect's inliner interface to no longer be registered at dialect initialization. Instead, it is now a promised interface, that needs to be registered explicitly. This change is desired to avoid pulling in a lot of dependencies into the MLIRLLVMDialect library, especially considering future patches that plan to extend it further with strong IR analysis.

This commit changes the LLVM dialect's inliner interface to no longer be registered at dialect initialization. Instead, it is now a promised interface, that needs to be registered explicitly. This change is desired to avoid pulling in a lot of dependencies into the `MLIRLLVMDialect` library, especially considering future patches that plan to extend it further with strong IR analysis.

llvmbot · 2024-08-14T13:15:39Z

@llvm/pr-subscribers-flang-fir-hlfir
@llvm/pr-subscribers-mlir-llvm

@llvm/pr-subscribers-mlir

Author: Christian Ulmann (Dinistro)

Changes

This commit changes the LLVM dialect's inliner interface to no longer be registered at dialect initialization. Instead, it is now a promised interface, that needs to be registered explicitly. This change is desired to avoid pulling in a lot of dependencies into the MLIRLLVMDialect library, especially considering future patches that plan to extend it further with strong IR analysis.

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

7 Files Affected:

(modified) flang/include/flang/Optimizer/Support/InitFIR.h (+1)
(renamed) mlir/include/mlir/Dialect/LLVMIR/Transforms/InlinerInterfaceImpl.h (+8-13)
(modified) mlir/include/mlir/InitAllDialects.h (+2)
(modified) mlir/lib/Dialect/LLVMIR/CMakeLists.txt (-1)
(modified) mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp (+2-2)
(modified) mlir/lib/Dialect/LLVMIR/Transforms/CMakeLists.txt (+1)
(renamed) mlir/lib/Dialect/LLVMIR/Transforms/InlinerInterfaceImpl.cpp (+6-4)

diff --git a/flang/include/flang/Optimizer/Support/InitFIR.h b/flang/include/flang/Optimizer/Support/InitFIR.h
index 48cc1cbc645684..0192d7fdbea099 100644
--- a/flang/include/flang/Optimizer/Support/InitFIR.h
+++ b/flang/include/flang/Optimizer/Support/InitFIR.h
@@ -46,6 +46,7 @@ namespace fir::support {
 inline void registerNonCodegenDialects(mlir::DialectRegistry &registry) {
   registry.insert<FLANG_NONCODEGEN_DIALECT_LIST>();
   mlir::func::registerInlinerExtension(registry);
+  mlir::LLVM::registerLLVMInlinerInterface(registry);
 }
 
 /// Register all the dialects used by flang.
diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMInlining.h b/mlir/include/mlir/Dialect/LLVMIR/Transforms/InlinerInterfaceImpl.h
similarity index 64%
rename from mlir/lib/Dialect/LLVMIR/IR/LLVMInlining.h
rename to mlir/include/mlir/Dialect/LLVMIR/Transforms/InlinerInterfaceImpl.h
index c6f75d5657c3be..9c94cf6512195f 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMInlining.h
+++ b/mlir/include/mlir/Dialect/LLVMIR/Transforms/InlinerInterfaceImpl.h
@@ -1,4 +1,4 @@
-//===- LLVMInlining.h - Registration of LLVMInlinerInterface ----*- C++ -*-===//
+//===- InlinerInterfaceImpl.h - Inlining for LLVM the dialect ---*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -6,28 +6,23 @@
 //
 //===----------------------------------------------------------------------===//
 //
-// Allows registering the LLVM DialectInlinerInterface with the LLVM dialect
-// during initialization.
+// Allows registering the LLVM DialectInlinerInterface with the LLVM dialect.
 //
 //===----------------------------------------------------------------------===//
 
-#ifndef DIALECT_LLVMIR_IR_LLVMINLINING_H
-#define DIALECT_LLVMIR_IR_LLVMINLINING_H
+#ifndef MLIR_DIALECT_LLVMIR_TRANSFORMS_INLINERINTERFACEIMPL_H
+#define MLIR_DIALECT_LLVMIR_TRANSFORMS_INLINERINTERFACEIMPL_H
 
 namespace mlir {
-namespace LLVM {
-
-class LLVMDialect;
+class DialectRegistry;
 
-namespace detail {
+namespace LLVM {
 
 /// Register the `LLVMInlinerInterface` implementation of
 /// `DialectInlinerInterface` with the LLVM dialect.
-void addLLVMInlinerInterface(LLVMDialect *dialect);
-
-} // namespace detail
+void registerLLVMInlinerInterface(DialectRegistry &registry);
 
 } // namespace LLVM
 } // namespace mlir
 
-#endif // DIALECT_LLVMIR_IR_LLVMINLINING_H
+#endif // MLIR_DIALECT_LLVMIR_TRANSFORMS_INLINERINTERFACEIMPL_H
diff --git a/mlir/include/mlir/InitAllDialects.h b/mlir/include/mlir/InitAllDialects.h
index 549c26c72d8a1e..cdece616e4cf7c 100644
--- a/mlir/include/mlir/InitAllDialects.h
+++ b/mlir/include/mlir/InitAllDialects.h
@@ -43,6 +43,7 @@
 #include "mlir/Dialect/LLVMIR/LLVMDialect.h"
 #include "mlir/Dialect/LLVMIR/NVVMDialect.h"
 #include "mlir/Dialect/LLVMIR/ROCDLDialect.h"
+#include "mlir/Dialect/LLVMIR/Transforms/InlinerInterfaceImpl.h"
 #include "mlir/Dialect/Linalg/IR/Linalg.h"
 #include "mlir/Dialect/Linalg/Transforms/AllInterfaces.h"
 #include "mlir/Dialect/Linalg/Transforms/RuntimeOpVerification.h"
@@ -163,6 +164,7 @@ inline void registerAllDialects(DialectRegistry &registry) {
   cf::registerBufferizableOpInterfaceExternalModels(registry);
   cf::registerBufferDeallocationOpInterfaceExternalModels(registry);
   gpu::registerBufferDeallocationOpInterfaceExternalModels(registry);
+  LLVM::registerLLVMInlinerInterface(registry);
   linalg::registerAllDialectInterfaceImplementations(registry);
   linalg::registerRuntimeVerifiableOpInterfaceExternalModels(registry);
   memref::registerAllocationOpInterfaceExternalModels(registry);
diff --git a/mlir/lib/Dialect/LLVMIR/CMakeLists.txt b/mlir/lib/Dialect/LLVMIR/CMakeLists.txt
index 392065b859ee54..fce24b556036f3 100644
--- a/mlir/lib/Dialect/LLVMIR/CMakeLists.txt
+++ b/mlir/lib/Dialect/LLVMIR/CMakeLists.txt
@@ -4,7 +4,6 @@ add_mlir_dialect_library(MLIRLLVMDialect
   IR/FunctionCallUtils.cpp
   IR/LLVMAttrs.cpp
   IR/LLVMDialect.cpp
-  IR/LLVMInlining.cpp
   IR/LLVMInterfaces.cpp
   IR/LLVMMemorySlot.cpp
   IR/LLVMTypes.cpp
diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
index 90610118a45cd2..476281d44e4306 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
@@ -12,7 +12,6 @@
 //===----------------------------------------------------------------------===//
 
 #include "mlir/Dialect/LLVMIR/LLVMDialect.h"
-#include "LLVMInlining.h"
 #include "TypeDetail.h"
 #include "mlir/Dialect/LLVMIR/LLVMAttrs.h"
 #include "mlir/Dialect/LLVMIR/LLVMInterfaces.h"
@@ -24,6 +23,7 @@
 #include "mlir/IR/MLIRContext.h"
 #include "mlir/IR/Matchers.h"
 #include "mlir/Interfaces/FunctionImplementation.h"
+#include "mlir/Transforms/InliningUtils.h"
 
 #include "llvm/ADT/SCCIterator.h"
 #include "llvm/ADT/TypeSwitch.h"
@@ -3200,7 +3200,7 @@ void LLVMDialect::initialize() {
   // clang-format off
   addInterfaces<LLVMOpAsmDialectInterface>();
   // clang-format on
-  detail::addLLVMInlinerInterface(this);
+  declarePromisedInterface<DialectInlinerInterface, LLVMDialect>();
 }
 
 #define GET_OP_CLASSES
diff --git a/mlir/lib/Dialect/LLVMIR/Transforms/CMakeLists.txt b/mlir/lib/Dialect/LLVMIR/Transforms/CMakeLists.txt
index 728885fcbeaf38..d4ff0955c5d0e2 100644
--- a/mlir/lib/Dialect/LLVMIR/Transforms/CMakeLists.txt
+++ b/mlir/lib/Dialect/LLVMIR/Transforms/CMakeLists.txt
@@ -3,6 +3,7 @@ add_mlir_dialect_library(MLIRLLVMIRTransforms
   DIExpressionLegalization.cpp
   DIExpressionRewriter.cpp
   DIScopeForLLVMFuncOp.cpp
+  InlinerInterfaceImpl.cpp
   LegalizeForExport.cpp
   OptimizeForNVVM.cpp
   RequestCWrappers.cpp
diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMInlining.cpp b/mlir/lib/Dialect/LLVMIR/Transforms/InlinerInterfaceImpl.cpp
similarity index 99%
rename from mlir/lib/Dialect/LLVMIR/IR/LLVMInlining.cpp
rename to mlir/lib/Dialect/LLVMIR/Transforms/InlinerInterfaceImpl.cpp
index 137c1962b100af..55bf99f4228ae4 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMInlining.cpp
+++ b/mlir/lib/Dialect/LLVMIR/Transforms/InlinerInterfaceImpl.cpp
@@ -1,4 +1,4 @@
-//===- LLVMInlining.cpp - LLVM inlining interface and logic -----*- C++ -*-===//
+//===- InlinerInterfaceImpl.cpp - Inlining for LLVM the dialect -----------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -11,7 +11,7 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "LLVMInlining.h"
+#include "mlir/Dialect/LLVMIR/Transforms/InlinerInterfaceImpl.h"
 #include "mlir/Dialect/LLVMIR/LLVMDialect.h"
 #include "mlir/IR/Matchers.h"
 #include "mlir/Interfaces/DataLayoutInterfaces.h"
@@ -850,6 +850,8 @@ struct LLVMInlinerInterface : public DialectInlinerInterface {
 
 } // end anonymous namespace
 
-void LLVM::detail::addLLVMInlinerInterface(LLVM::LLVMDialect *dialect) {
-  dialect->addInterfaces<LLVMInlinerInterface>();
+void mlir::LLVM::registerLLVMInlinerInterface(DialectRegistry &registry) {
+  registry.addExtension(+[](MLIRContext *ctx, LLVM::LLVMDialect *dialect) {
+    dialect->addInterfaces<LLVMInlinerInterface>();
+  });
 }

gysit

LGTM

gysit · 2024-08-14T13:34:12Z

mlir/include/mlir/Dialect/LLVMIR/Transforms/InlinerInterfaceImpl.h

-void addLLVMInlinerInterface(LLVMDialect *dialect);
-
-} // namespace detail
+void registerLLVMInlinerInterface(DialectRegistry &registry);


Suggested change

void registerLLVMInlinerInterface(DialectRegistry &registry);

void registerInlinerInterface(DialectRegistry &registry);

Nit: since it is in the LLVM namespace it may make sense to drop it in the function name?

definelicht · 2024-08-14T13:39:28Z

mlir/include/mlir/Dialect/LLVMIR/Transforms/InlinerInterfaceImpl.h

The Impl naming seems weird, especially in a public header file?

That seems to be the standard for such kind of things.

definelicht

LGTM

llvm-ci · 2024-08-14T16:37:00Z

LLVM Buildbot has detected a new failure on builder mlir-nvidia-gcc7 running on mlir-nvidia while building flang,mlir at step 5 "build-check-mlir-build-only".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/116/builds/2372

Here is the relevant piece of the build log for the reference:

Step 5 (build-check-mlir-build-only) failure: build (failure)
...
/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/mlir/lib/Bindings/Python/IRModule.h: In instantiation of ‘static void mlir::python::PyConcreteType<DerivedTy, BaseTy>::bind(pybind11::module&) [with DerivedTy = {anonymous}::PyOpaqueType; BaseTy = mlir::python::PyType; pybind11::module = pybind11::module_]’:
/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/mlir/lib/Bindings/Python/IRTypes.cpp:903:17:   required from here
/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/mlir/lib/Bindings/Python/IRModule.h:986:5: warning: the address of ‘MlirTypeID mlirOpaqueTypeGetTypeID()’ will never be NULL [-Waddress]
     if (DerivedTy::getTypeIdFunction) {
     ^~
39.425 [323/16/4174] Building CXX object tools/mlir/python/CMakeFiles/MLIRPythonModules.extension._mlir.dso.dir/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/mlir/lib/Bindings/Python/Pass.cpp.o
39.456 [322/16/4175] Building CXX object tools/mlir/python/CMakeFiles/MLIRPythonModules.extension._mlir.dso.dir/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/mlir/lib/Bindings/Python/Rewrite.cpp.o
39.485 [321/16/4176] Building CXX object tools/mlir/python/CMakeFiles/MLIRPythonModules.extension._mlirExecutionEngine.dso.dir/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/mlir/lib/Bindings/Python/ExecutionEngineModule.cpp.o
39.513 [320/16/4177] Building CXX object tools/mlir/python/CMakeFiles/MLIRPythonModules.extension._mlirRegisterEverything.dso.dir/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/mlir/lib/Bindings/Python/RegisterEverything.cpp.o
39.544 [319/16/4178] Building CXX object tools/mlir/python/CMakeFiles/MLIRPythonModules.extension._mlirPythonTest.dso.dir/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/mlir/test/python/lib/PythonTestModule.cpp.o
command timed out: 1200 seconds without output running [b'ninja', b'-j', b'16', b'check-mlir-build-only'], attempting to kill
process killed by signal 9
program finished with exit code -1
elapsedTime=2412.979753

Dinistro · 2024-08-14T16:46:16Z

LLVM Buildbot has detected a new failure on builder mlir-nvidia-gcc7 running on mlir-nvidia while building flang,mlir at step 5 "build-check-mlir-build-only".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/116/builds/2372

Here is the relevant piece of the build log for the reference:

Step 5 (build-check-mlir-build-only) failure: build (failure)
...
/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/mlir/lib/Bindings/Python/IRModule.h: In instantiation of ‘static void mlir::python::PyConcreteType<DerivedTy, BaseTy>::bind(pybind11::module&) [with DerivedTy = {anonymous}::PyOpaqueType; BaseTy = mlir::python::PyType; pybind11::module = pybind11::module_]’:
/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/mlir/lib/Bindings/Python/IRTypes.cpp:903:17:   required from here
/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/mlir/lib/Bindings/Python/IRModule.h:986:5: warning: the address of ‘MlirTypeID mlirOpaqueTypeGetTypeID()’ will never be NULL [-Waddress]
     if (DerivedTy::getTypeIdFunction) {
     ^~
39.425 [323/16/4174] Building CXX object tools/mlir/python/CMakeFiles/MLIRPythonModules.extension._mlir.dso.dir/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/mlir/lib/Bindings/Python/Pass.cpp.o
39.456 [322/16/4175] Building CXX object tools/mlir/python/CMakeFiles/MLIRPythonModules.extension._mlir.dso.dir/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/mlir/lib/Bindings/Python/Rewrite.cpp.o
39.485 [321/16/4176] Building CXX object tools/mlir/python/CMakeFiles/MLIRPythonModules.extension._mlirExecutionEngine.dso.dir/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/mlir/lib/Bindings/Python/ExecutionEngineModule.cpp.o
39.513 [320/16/4177] Building CXX object tools/mlir/python/CMakeFiles/MLIRPythonModules.extension._mlirRegisterEverything.dso.dir/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/mlir/lib/Bindings/Python/RegisterEverything.cpp.o
39.544 [319/16/4178] Building CXX object tools/mlir/python/CMakeFiles/MLIRPythonModules.extension._mlirPythonTest.dso.dir/vol/worker/mlir-nvidia/mlir-nvidia-gcc7/llvm.src/mlir/test/python/lib/PythonTestModule.cpp.o
command timed out: 1200 seconds without output running [b'ninja', b'-j', b'16', b'check-mlir-build-only'], attempting to kill
process killed by signal 9
program finished with exit code -1
elapsedTime=2412.979753

This seems to be a build timeout and should not be related to this PR.

llvm-ci · 2024-08-14T18:04:27Z

LLVM Buildbot has detected a new failure on builder mlir-nvidia running on mlir-nvidia while building flang,mlir at step 2 "checkout".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/138/builds/2442

Here is the relevant piece of the build log for the reference:

Step 2 (checkout) failure: update (failure)
command timed out: 1200 seconds without output running [b'git', b'--version'], attempting to kill
process killed by signal 15
program finished with exit code -1
elapsedTime=1639.782699

See llvm/llvm-project#103927.

Dinistro requested review from definelicht, kiranchandramohan and gysit August 14, 2024 13:15

llvmbot added mlir:llvm mlir flang Flang issues not falling into any other category flang:fir-hlfir labels Aug 14, 2024

Dinistro mentioned this pull request Aug 14, 2024

[MLIR][LLVM]: Add an IR utility to perform slice walking #103053

Merged

gysit approved these changes Aug 14, 2024

View reviewed changes

definelicht reviewed Aug 14, 2024

View reviewed changes

address review comments

a134c85

definelicht approved these changes Aug 14, 2024

View reviewed changes

register interface in the examples

6f68976

Dinistro merged commit 4c2f90f into main Aug 14, 2024
6 of 7 checks passed

Dinistro deleted the users/dinistro/llvm-inlining-interface-move branch August 14, 2024 15:29

fabianschuiki added a commit to llvm/circt that referenced this pull request Sep 27, 2024

[arcilator] Add missing LLVM registerInlinerInterface

4177849

See llvm/llvm-project#103927.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[MLIR][LLVM] Turn the inliner interface into a promised interface #103927

[MLIR][LLVM] Turn the inliner interface into a promised interface #103927

Uh oh!

Dinistro commented Aug 14, 2024

Uh oh!

llvmbot commented Aug 14, 2024 •

edited

Loading

Uh oh!

gysit left a comment

Uh oh!

gysit Aug 14, 2024

Uh oh!

definelicht Aug 14, 2024

Uh oh!

Dinistro Aug 14, 2024

Uh oh!

definelicht left a comment

Uh oh!

Uh oh!

llvm-ci commented Aug 14, 2024

Uh oh!

Dinistro commented Aug 14, 2024

Uh oh!

llvm-ci commented Aug 14, 2024

Uh oh!

Uh oh!

	void registerLLVMInlinerInterface(DialectRegistry &registry);
	void registerInlinerInterface(DialectRegistry &registry);

[MLIR][LLVM] Turn the inliner interface into a promised interface #103927

[MLIR][LLVM] Turn the inliner interface into a promised interface #103927

Uh oh!

Conversation

Dinistro commented Aug 14, 2024

Uh oh!

llvmbot commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gysit left a comment

Choose a reason for hiding this comment

Uh oh!

gysit Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

definelicht Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

Dinistro Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

definelicht left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvm-ci commented Aug 14, 2024

Uh oh!

Dinistro commented Aug 14, 2024

Uh oh!

llvm-ci commented Aug 14, 2024

Uh oh!

Uh oh!

llvmbot commented Aug 14, 2024 •

edited

Loading