Skip to content
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

[MLIR] Add missing MLIRDialectUtils dep to TilingInterface #84544

Merged
merged 1 commit into from
Mar 20, 2024

Conversation

RoboTux
Copy link
Contributor

@RoboTux RoboTux commented Mar 8, 2024

This fixes the following failure when doing a clean build (in particular
no .ninja* lying around) of lib/libMLIRTilingInterface.a only:

In file included from mlir/include/mlir/Interfaces/TilingInterface.h:17,                                                                     
                 from mlir/lib/Interfaces/TilingInterface.cpp:13:                                                                            
mlir/include/mlir/Dialect/Utils/StructuredOpsUtils.h:27:10: fatal error: mlir/Dialect/Utils/DialectUtilsEnums.h.inc: No such file or directory

This fixes a failure when doing a clean build of
lib/libMLIRDialectUtils.a only.
@llvmbot llvmbot added the mlir label Mar 8, 2024
@llvmbot
Copy link
Member

llvmbot commented Mar 8, 2024

@llvm/pr-subscribers-mlir

Author: Thomas Preud'homme (RoboTux)

Changes

This fixes a failure when doing a clean build of
lib/libMLIRDialectUtils.a only.


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

1 Files Affected:

  • (modified) mlir/lib/Interfaces/CMakeLists.txt (+14-1)
diff --git a/mlir/lib/Interfaces/CMakeLists.txt b/mlir/lib/Interfaces/CMakeLists.txt
index e7c76e70ed6b5d..d3b7bf65ad3e73 100644
--- a/mlir/lib/Interfaces/CMakeLists.txt
+++ b/mlir/lib/Interfaces/CMakeLists.txt
@@ -101,7 +101,20 @@ add_mlir_library(MLIRSubsetOpInterface
   MLIRValueBoundsOpInterface
   )
 
-add_mlir_interface_library(TilingInterface)
+add_mlir_library(MLIRTilingInterface
+  TilingInterface.cpp
+
+  ADDITIONAL_HEADER_DIRS
+  ${MLIR_MAIN_INCLUDE_DIR}/mlir/Interfaces
+
+  DEPENDS
+  MLIRTilingInterfaceIncGen
+  MLIRDialectUtils
+
+  LINK_LIBS PUBLIC
+  MLIRIR
+)
+
 add_mlir_interface_library(VectorInterfaces)
 add_mlir_interface_library(ViewLikeInterface)
 

@RoboTux RoboTux changed the title [MLIR] Add missing TilingInterface dep on MLIRDialectUtils [MLIR] Add missing MLIRDialectUtils dep to TilingInterface Mar 10, 2024
@RoboTux
Copy link
Contributor Author

RoboTux commented Mar 19, 2024

Ping?

@matthias-springer
Copy link
Member

This change is problematic because it introduces a dependency from mlir/Interfaces on a dialect. (DialectUtils depends on ArithDialect.) I'd like to understand why TilingInterface needs DialectUtils. Do you see a way to remove that dependency?

@RoboTux
Copy link
Contributor Author

RoboTux commented Mar 20, 2024

This change is problematic because it introduces a dependency from mlir/Interfaces on a dialect. (DialectUtils depends on ArithDialect.) I'd like to understand why TilingInterface needs DialectUtils. Do you see a way to remove that dependency?

DialectUtils provide the IteratorType used in TilingInterface's getLoopIteratorTypes() return type(SmallVectorutils::IteratorType).

On the other hand AFAIK the ArithDialect dependency is only because of this line:
https://github.com/llvm/llvm-project/blob/main/mlir/lib/Dialect/Utils/StaticValueUtils.cpp#L25

(it also need an extra include of mlir/IR/Builders.h in StaticValueUtils.h and a MLIRDialectUtilsIncGen dependency in MLIRDialectUtils library definition). Could that somehow be made conditional upon ArithDialect being linked transitively or maybe moved to another place?

@matthias-springer
Copy link
Member

If that's the only reason, we should be able to replace the implementation with a call to getConstantIntValue, which uses the matcher API. Then we won't rely on the Arith dialect anymore.

@RoboTux
Copy link
Contributor Author

RoboTux commented Mar 20, 2024

Perfect, I'll make a patch to dissociate UtilsDialect from ArithUtils.

@RoboTux
Copy link
Contributor Author

RoboTux commented Mar 20, 2024

Perfect, I'll make a patch to dissociate UtilsDialect from ArithUtils.

#85919

@RoboTux RoboTux merged commit caf8b1f into llvm:main Mar 20, 2024
6 checks passed
chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024
This fixes the following failure when doing a clean build (in particular
no .ninja* lying around) of lib/libMLIRTilingInterface.a only:
```
In file included from mlir/include/mlir/Interfaces/TilingInterface.h:17,                                                                     
                 from mlir/lib/Interfaces/TilingInterface.cpp:13:                                                                            
mlir/include/mlir/Dialect/Utils/StructuredOpsUtils.h:27:10: fatal error: mlir/Dialect/Utils/DialectUtilsEnums.h.inc: No such file or directory
```
@RoboTux RoboTux deleted the tilinginterface_missing_dialectutils branch April 18, 2024 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants