Skip to content

[mlir] Fix some cmake dependencies in LLVMIR Dialect #66956

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 22, 2023
Merged

[mlir] Fix some cmake dependencies in LLVMIR Dialect #66956

merged 3 commits into from
Sep 22, 2023

Conversation

zero9178
Copy link
Member

While looking into reducing needless interdependencies between upstream MLIR dialects and passes, I discovered that the ROCDL Dialect redundantely uses links in VectorToLLVM conversion pass when it actually requires just the LLVM Dialect. Furthermore, after a build failure, I ran ninja -t missingdeps which revealed that the NVVM Dialect depends on headers of the GPU dialect (

llvm-project/mlir/include/mlir/Dialect/LLVMIR/NVVMDialect.h

Line 18 in 211c975

#include "mlir/Dialect/GPU/IR/CompilationInterfaces.h"
) without stating so in CMake.
This causes flaky builds as it is not guaranteed that the header exists prior to the dialect being compiled.

@zero9178
[mlir] Fix some cmake dependencies in LLVMIR Dialect
b9734d3 
While looking into reducing needless interdependencies between upstream MLIR dialects and passes, I discovered that the ROCDL Dialect redundantely uses links in `VectorToLLVM` conversion pass when it actually requires just the LLVM Dialect.
Furthermore, after a build failure, I ran `ninja -t missingdeps` which revealed that the NVVM Dialect depends on headers of the GPU dialect without stating so in CMake. This causes flaky builds as it is not guaranteed that the header exists prior to the dialect being compiled.
@llvmbot
Copy link
Member

llvmbot commented Sep 20, 2023 •
edited
Loading

@llvm/pr-subscribers-mlir-llvm

@llvm/pr-subscribers-mlir

Changes

While looking into reducing needless interdependencies between upstream MLIR dialects and passes, I discovered that the ROCDL Dialect redundantely uses links in VectorToLLVM conversion pass when it actually requires just the LLVM Dialect. Furthermore, after a build failure, I ran ninja -t missingdeps which revealed that the NVVM Dialect depends on headers of the GPU dialect (

llvm-project/mlir/include/mlir/Dialect/LLVMIR/NVVMDialect.h

Line 18 in 211c975

#include "mlir/Dialect/GPU/IR/CompilationInterfaces.h"
) without stating so in CMake.
This causes flaky builds as it is not guaranteed that the header exists prior to the dialect being compiled.

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

1 Files Affected:

  • (modified) mlir/lib/Dialect/LLVMIR/CMakeLists.txt (+2-1)
diff --git a/mlir/lib/Dialect/LLVMIR/CMakeLists.txt b/mlir/lib/Dialect/LLVMIR/CMakeLists.txt
index b5e6fbd4baf6ba7..b54a1e211d08c91 100644
--- a/mlir/lib/Dialect/LLVMIR/CMakeLists.txt
+++ b/mlir/lib/Dialect/LLVMIR/CMakeLists.txt
@@ -57,6 +57,7 @@ add_mlir_dialect_library(MLIRNVVMDialect
 
   LINK_LIBS PUBLIC
   MLIRIR
+  MLIRGPUDialect
   MLIRLLVMDialect
   MLIRSideEffectInterfaces
   )
@@ -78,6 +79,6 @@ add_mlir_dialect_library(MLIRROCDLDialect
 
   LINK_LIBS PUBLIC
   MLIRIR
+  MLIRLLVMDialect
   MLIRSideEffectInterfaces
-  MLIRVectorToLLVM
   )

@fabianmcg
Copy link
Contributor

Could you try changing the NVVM part to this:

diff --git a/mlir/lib/Dialect/LLVMIR/CMakeLists.txt b/mlir/lib/Dialect/LLVMIR/CMakeLists.txt
index b5e6fbd4baf6..66a38d8ea4a1 100644
--- a/mlir/lib/Dialect/LLVMIR/CMakeLists.txt
+++ b/mlir/lib/Dialect/LLVMIR/CMakeLists.txt
@@ -49,6 +49,7 @@ add_mlir_dialect_library(MLIRNVVMDialect
   DEPENDS
   MLIRNVVMOpsIncGen
   MLIRNVVMConversionsIncGen
+  MLIRGPUCompilationAttrInterfacesIncGen
   intrinsics_gen
 
   LINK_COMPONENTS
@@ -70,6 +71,7 @@ add_mlir_dialect_library(MLIRROCDLDialect
   DEPENDS
   MLIRROCDLOpsIncGen
   MLIRROCDLConversionsIncGen
+  MLIRGPUCompilationAttrInterfacesIncGen
   intrinsics_gen
 
   LINK_COMPONENTS
diff --git a/mlir/lib/Dialect/LLVMIR/IR/NVVMDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/NVVMDialect.cpp
index 76c043e41ea8..9033194f0fec 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/NVVMDialect.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/NVVMDialect.cpp
@@ -17,7 +17,6 @@
 #include "mlir/Dialect/LLVMIR/NVVMDialect.h"
 
 #include "mlir/Conversion/ConvertToLLVM/ToLLVMInterface.h"
-#include "mlir/Dialect/GPU/IR/GPUDialect.h"
 #include "mlir/Dialect/Utils/StaticValueUtils.h"
 #include "mlir/IR/Builders.h"
 #include "mlir/IR/BuiltinAttributes.h"
diff --git a/mlir/lib/Dialect/LLVMIR/IR/ROCDLDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/ROCDLDialect.cpp
index 32f34a8889af..a9e829716953 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/ROCDLDialect.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/ROCDLDialect.cpp
@@ -16,7 +16,6 @@
 
 #include "mlir/Dialect/LLVMIR/ROCDLDialect.h"
 
-#include "mlir/Dialect/GPU/IR/GPUDialect.h"
 #include "mlir/Dialect/LLVMIR/LLVMDialect.h"
 #include "mlir/IR/Builders.h"
 #include "mlir/IR/BuiltinTypes.h"

NVVM & ROCDL don't require the full GPUDialect to be linked, only the header created by MLIRGPUCompilationAttrInterfacesIncGen to get generated .

@zero9178
Copy link
Member Author

Could you try changing the NVVM part to this:

diff --git a/mlir/lib/Dialect/LLVMIR/CMakeLists.txt b/mlir/lib/Dialect/LLVMIR/CMakeLists.txt
index b5e6fbd4baf6..66a38d8ea4a1 100644
--- a/mlir/lib/Dialect/LLVMIR/CMakeLists.txt
+++ b/mlir/lib/Dialect/LLVMIR/CMakeLists.txt
@@ -49,6 +49,7 @@ add_mlir_dialect_library(MLIRNVVMDialect
   DEPENDS
   MLIRNVVMOpsIncGen
   MLIRNVVMConversionsIncGen
+  MLIRGPUCompilationAttrInterfacesIncGen
   intrinsics_gen
 
   LINK_COMPONENTS
@@ -70,6 +71,7 @@ add_mlir_dialect_library(MLIRROCDLDialect
   DEPENDS
   MLIRROCDLOpsIncGen
   MLIRROCDLConversionsIncGen
+  MLIRGPUCompilationAttrInterfacesIncGen
   intrinsics_gen
 
   LINK_COMPONENTS
diff --git a/mlir/lib/Dialect/LLVMIR/IR/NVVMDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/NVVMDialect.cpp
index 76c043e41ea8..9033194f0fec 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/NVVMDialect.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/NVVMDialect.cpp
@@ -17,7 +17,6 @@
 #include "mlir/Dialect/LLVMIR/NVVMDialect.h"
 
 #include "mlir/Conversion/ConvertToLLVM/ToLLVMInterface.h"
-#include "mlir/Dialect/GPU/IR/GPUDialect.h"
 #include "mlir/Dialect/Utils/StaticValueUtils.h"
 #include "mlir/IR/Builders.h"
 #include "mlir/IR/BuiltinAttributes.h"
diff --git a/mlir/lib/Dialect/LLVMIR/IR/ROCDLDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/ROCDLDialect.cpp
index 32f34a8889af..a9e829716953 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/ROCDLDialect.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/ROCDLDialect.cpp
@@ -16,7 +16,6 @@
 
 #include "mlir/Dialect/LLVMIR/ROCDLDialect.h"
 
-#include "mlir/Dialect/GPU/IR/GPUDialect.h"
 #include "mlir/Dialect/LLVMIR/LLVMDialect.h"
 #include "mlir/IR/Builders.h"
 #include "mlir/IR/BuiltinTypes.h"

NVVM & ROCDL don't require the full GPUDialect to be linked, only the header created by MLIRGPUCompilationAttrInterfacesIncGen to get generated .

Done. I've also gone ahead and changed the "include"s in both files to only include the header for the compilation interface that is purely used for the promised interface as far as I can tell.

@joker-eph
Copy link
Collaborator

Why is the dependency on the LLVM dialect needed?

@zero9178
Copy link
Member Author

Why is the dependency on the LLVM dialect needed?

The ROCDL dialect uses the LLVMFixedVectorType type from the LLVM Dialect and has one verifier depending on LLVMs FuncOp. It previously had this dependency as well but got it transitively through the VectorToLLVM dependency.

fabianmcg
fabianmcg approved these changes Sep 21, 2023
View reviewed changes
Copy link
Contributor

@fabianmcg fabianmcg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right it's only needed for the promise, could you also remove #include "mlir/Dialect/GPU/IR/CompilationInterfaces.h" from include/mlir/Dialect/LLVMIR/(NVVM|ROCDL)Dialect.h so they don't inadvertently pollute other files. Thank you for catching these.

@zero9178
Copy link
Member Author

@joker-eph do you have any more concerns or is it okay to land?

@zero9178 zero9178 merged commit 9779a73 into llvm:main Sep 22, 2023
@zero9178 zero9178 deleted the cmake-dependencies branch September 22, 2023 21:32
@kstoimenov kstoimenov mentioned this pull request Sep 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpienaar jpienaar jpienaar approved these changes

@fabianmcg fabianmcg fabianmcg approved these changes

@ftynse ftynse Awaiting requested review from ftynse

@joker-eph joker-eph Awaiting requested review from joker-eph

Assignees
No one assigned
Labels
mlir:llvm mlir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@zero9178 @llvmbot @fabianmcg @joker-eph @jpienaar