[mlir] Consider mlir-linalg-ods-gen as a tablegen tool in build #75093

MikeHolman · 2023-12-11T20:18:59Z

There is a bit of an issue with how mlir-linalg-ods-yaml-gen is classified in the MLIR build. Due to it being a tool, it is excluded from the install when using -DLLVM_BUILD_TOOLS=OFF. However, it is a necessary component of the build, so it can cause build issues with users of the installed LLVM, and so I think it should not be excluded.

It is a tablegen-like tool, so my solution is to reclassify it that way in the build.

llvmbot · 2023-12-11T20:19:27Z

@llvm/pr-subscribers-mlir-linalg
@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-core

Author: Michael Holman (MikeHolman)

Changes

There is a bit of an issue with how mlir-linalg-ods-yaml-gen is classified in the MLIR build. Due to it being a tool, it is excluded from the install when using -DLLVM_BUILD_TOOLS=OFF. However, it is a necessary component of the build, so it can cause build issues with users of the installed LLVM, and so I think it should not be excluded.

It is a tablegen-like tool, so my solution is to reclassify it that way in the build.

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

8 Files Affected:

(modified) llvm/cmake/modules/TableGen.cmake (+8-4)
(modified) mlir/CMakeLists.txt (+2)
(modified) mlir/cmake/modules/AddMLIR.cmake (+25)
(modified) mlir/cmake/modules/CMakeLists.txt (+2)
(modified) mlir/cmake/modules/MLIRConfig.cmake.in (+1)
(modified) mlir/include/mlir/Dialect/Linalg/IR/CMakeLists.txt (+1-34)
(modified) mlir/lib/Dialect/Linalg/IR/CMakeLists.txt (+1)
(modified) mlir/tools/mlir-linalg-ods-gen/CMakeLists.txt (+14-14)

diff --git a/llvm/cmake/modules/TableGen.cmake b/llvm/cmake/modules/TableGen.cmake
index 1d18fdde2bb98..359d5217a22cb 100644
--- a/llvm/cmake/modules/TableGen.cmake
+++ b/llvm/cmake/modules/TableGen.cmake
@@ -165,14 +165,18 @@ function(add_public_tablegen_target target)
 endfunction()
 
 macro(add_tablegen target project)
-  cmake_parse_arguments(ADD_TABLEGEN "" "DESTINATION;EXPORT" "" ${ARGN})
+  cmake_parse_arguments(ADD_TABLEGEN "SKIP_COMPONENT_LINK" "DESTINATION;EXPORT" "" ${ARGN})
 
-  set(${target}_OLD_LLVM_LINK_COMPONENTS ${LLVM_LINK_COMPONENTS})
-  set(LLVM_LINK_COMPONENTS ${LLVM_LINK_COMPONENTS} TableGen)
+  if(NOT ADD_TABLEGEN_SKIP_COMPONENT_LINK)
+    set(${target}_OLD_LLVM_LINK_COMPONENTS ${LLVM_LINK_COMPONENTS})
+    set(LLVM_LINK_COMPONENTS ${LLVM_LINK_COMPONENTS} TableGen)
+  endif()
 
   add_llvm_executable(${target} DISABLE_LLVM_LINK_LLVM_DYLIB
     ${ADD_TABLEGEN_UNPARSED_ARGUMENTS})
-  set(LLVM_LINK_COMPONENTS ${${target}_OLD_LLVM_LINK_COMPONENTS})
+  if(NOT ADD_TABLEGEN_SKIP_COMPONENT_LINK)
+    set(LLVM_LINK_COMPONENTS ${${target}_OLD_LLVM_LINK_COMPONENTS})
+  endif()
 
   set(${project}_TABLEGEN_DEFAULT "${target}")
   if (LLVM_NATIVE_TOOL_DIR)
diff --git a/mlir/CMakeLists.txt b/mlir/CMakeLists.txt
index 16ff950089734..dcc068e4097c5 100644
--- a/mlir/CMakeLists.txt
+++ b/mlir/CMakeLists.txt
@@ -184,6 +184,8 @@ add_subdirectory(tools/mlir-pdll)
 
 set(MLIR_TABLEGEN_EXE "${MLIR_TABLEGEN_EXE}" CACHE INTERNAL "")
 set(MLIR_TABLEGEN_TARGET "${MLIR_TABLEGEN_TARGET}" CACHE INTERNAL "")
+set(MLIR_LINALG_ODS_YAML_GEN_TABLEGEN_EXE "${MLIR_LINALG_ODS_YAML_GEN_TABLEGEN_EXE}" CACHE INTERNAL "")
+set(MLIR_LINALG_ODS_YAML_GEN_TABLEGEN_TARGET "${MLIR_LINALG_ODS_YAML_GEN_TABLEGEN_TARGET}" CACHE INTERNAL "")
 set(MLIR_PDLL_TABLEGEN_EXE "${MLIR_PDLL_TABLEGEN_EXE}" CACHE INTERNAL "")
 set(MLIR_PDLL_TABLEGEN_TARGET "${MLIR_PDLL_TABLEGEN_TARGET}" CACHE INTERNAL "")
 
diff --git a/mlir/cmake/modules/AddMLIR.cmake b/mlir/cmake/modules/AddMLIR.cmake
index 1d2ed748bc2f1..971ea7eceff4c 100644
--- a/mlir/cmake/modules/AddMLIR.cmake
+++ b/mlir/cmake/modules/AddMLIR.cmake
@@ -136,6 +136,31 @@ function(add_mlir_pdll_library target inputFile ofn)
   add_public_tablegen_target(${target})
 endfunction()
 
+# Declare a function to generate ODS with mlir-linalg-ods-yaml-gen
+function(add_linalg_ods_yaml_gen yaml_ast_file output_file)
+  set(YAML_AST_SOURCE ${CMAKE_CURRENT_SOURCE_DIR}/${yaml_ast_file})
+  set(GEN_ODS_FILE ${CMAKE_CURRENT_BINARY_DIR}/${output_file}.yamlgen.td)
+  set(GEN_CPP_FILE ${CMAKE_CURRENT_BINARY_DIR}/${output_file}.yamlgen.cpp.inc)
+  set_source_files_properties(
+    ${GEN_ODS_FILE}
+    PROPERTIES GENERATED TRUE)
+  set_source_files_properties(
+    ${GEN_CPP_FILE}
+    PROPERTIES GENERATED TRUE)
+  add_custom_command(
+    OUTPUT ${GEN_ODS_FILE} ${GEN_CPP_FILE}
+    COMMAND ${MLIR_LINALG_ODS_YAML_GEN_TABLEGEN_EXE} ${YAML_AST_SOURCE} -o-ods-decl=${GEN_ODS_FILE} -o-impl=${GEN_CPP_FILE}
+    MAIN_DEPENDENCY
+    ${YAML_AST_SOURCE}
+    DEPENDS
+    ${MLIR_LINALG_ODS_YAML_GEN_TABLEGEN_EXE}
+    ${MLIR_LINALG_ODS_YAML_GEN_TABLEGEN_TARGET}
+    ${LLVM_TARGET_DEPENDS})
+    
+  set(TABLEGEN_OUTPUT ${TABLEGEN_OUTPUT} ${GEN_ODS_FILE} ${GEN_CPP_FILE}
+      PARENT_SCOPE)
+endfunction()
+
 # Declare a dialect in the include directory
 function(add_mlir_dialect dialect dialect_namespace)
   set(LLVM_TARGET_DEFINITIONS ${dialect}.td)
diff --git a/mlir/cmake/modules/CMakeLists.txt b/mlir/cmake/modules/CMakeLists.txt
index 8d2904ef46dfe..2b72beff89385 100644
--- a/mlir/cmake/modules/CMakeLists.txt
+++ b/mlir/cmake/modules/CMakeLists.txt
@@ -38,6 +38,7 @@ set(MLIR_CONFIG_INCLUDE_DIRS
   )
 # Refer to the best host mlir-tbgen, which might be a host-optimized version
 set(MLIR_CONFIG_TABLEGEN_EXE "${MLIR_TABLEGEN_EXE}")
+set(MLIR_CONFIG_LINALG_ODS_YAML_GEN_TABLEGEN_EXE "${MLIR_LINALG_ODS_YAML_GEN_TABLEGEN_EXE}")
 set(MLIR_CONFIG_PDLL_TABLEGEN_EXE "${MLIR_PDLL_TABLEGEN_EXE}")
 
 configure_file(
@@ -76,6 +77,7 @@ set(MLIR_CONFIG_INCLUDE_DIRS
 # Ensure that we are using the installed mlir-tblgen. This might not be MLIR_TABLEGEN_EXE
 # if we're building with a host-optimized mlir-tblgen (with LLVM_OPTIMIZED_TABLEGEN).
 set(MLIR_CONFIG_TABLEGEN_EXE mlir-tblgen)
+set(MLIR_CONFIG_LINALG_ODS_YAML_GEN_TABLEGEN_EXE mlir-linalg-ods-yaml-gen)
 set(MLIR_CONFIG_PDLL_TABLEGEN_EXE mlir-pdll)
 
 configure_file(
diff --git a/mlir/cmake/modules/MLIRConfig.cmake.in b/mlir/cmake/modules/MLIRConfig.cmake.in
index d4da3cd98cce9..0922b9accea5d 100644
--- a/mlir/cmake/modules/MLIRConfig.cmake.in
+++ b/mlir/cmake/modules/MLIRConfig.cmake.in
@@ -10,6 +10,7 @@ set(MLIR_EXPORTED_TARGETS "@MLIR_EXPORTS@")
 set(MLIR_CMAKE_DIR "@MLIR_CONFIG_CMAKE_DIR@")
 set(MLIR_INCLUDE_DIRS "@MLIR_CONFIG_INCLUDE_DIRS@")
 set(MLIR_TABLEGEN_EXE "@MLIR_CONFIG_TABLEGEN_EXE@")
+set(MLIR_LINALG_ODS_YAML_GEN_TABLEGEN_EXE "@MLIR_CONFIG_LINALG_ODS_YAML_GEN_TABLEGEN_EXE@")
 set(MLIR_PDLL_TABLEGEN_EXE "@MLIR_CONFIG_PDLL_TABLEGEN_EXE@")
 set(MLIR_INSTALL_AGGREGATE_OBJECTS "@MLIR_INSTALL_AGGREGATE_OBJECTS@")
 set(MLIR_ENABLE_BINDINGS_PYTHON "@MLIR_ENABLE_BINDINGS_PYTHON@")
diff --git a/mlir/include/mlir/Dialect/Linalg/IR/CMakeLists.txt b/mlir/include/mlir/Dialect/Linalg/IR/CMakeLists.txt
index f5d48b2ebcefe..c30665d4d118b 100644
--- a/mlir/include/mlir/Dialect/Linalg/IR/CMakeLists.txt
+++ b/mlir/include/mlir/Dialect/Linalg/IR/CMakeLists.txt
@@ -1,38 +1,5 @@
-# Declare a function to generate ODS with mlir-linalg-ods-yaml-gen
-function(add_linalg_ods_yaml_gen yaml_ast_file output_file)
-  set(YAML_AST_SOURCE ${CMAKE_CURRENT_SOURCE_DIR}/${yaml_ast_file})
-  set(GEN_ODS_FILE ${CMAKE_CURRENT_BINARY_DIR}/${output_file}.yamlgen.td)
-  set(GEN_CPP_FILE ${CMAKE_CURRENT_BINARY_DIR}/${output_file}.yamlgen.cpp.inc)
-  set_source_files_properties(
-    ${GEN_ODS_FILE}
-    PROPERTIES GENERATED TRUE)
-  set_source_files_properties(
-    ${GEN_CPP_FILE}
-    PROPERTIES GENERATED TRUE)
-  add_custom_command(
-    OUTPUT ${GEN_ODS_FILE} ${GEN_CPP_FILE}
-    COMMAND ${MLIR_LINALG_ODS_YAML_GEN_EXE} ${YAML_AST_SOURCE} -o-ods-decl=${GEN_ODS_FILE} -o-impl=${GEN_CPP_FILE}
-    MAIN_DEPENDENCY
-    ${YAML_AST_SOURCE}
-    DEPENDS
-    ${MLIR_LINALG_ODS_YAML_GEN_EXE}
-    ${MLIR_LINALG_ODS_YAML_GEN_TARGET})
-  add_custom_target(
-    MLIR${output_file}YamlIncGen
-    DEPENDS
-    ${MLIR_LINALG_ODS_YAML_GEN_EXE}
-    ${MLIR_LINALG_ODS_YAML_GEN_TARGET}
-    ${GEN_ODS_FILE} ${GEN_CPP_FILE})
-  list(APPEND LLVM_TARGET_DEPENDS ${GEN_ODS_FILE})
-  set(LLVM_TARGET_DEPENDS ${LLVM_TARGET_DEPENDS} PARENT_SCOPE)
-endfunction()
-
-# NOTE: LLVM_TARGET_DEPENDS gets picked up by tablegen targets to add file
-# level dependencies. This is gross but CMake requires depending on both
-# targets and generated files, and it must be done when the custom target is
-# declared (there is no way to add after the fact).
-set(LLVM_TARGET_DEPENDS "")
 add_linalg_ods_yaml_gen(LinalgNamedStructuredOps.yaml LinalgNamedStructuredOps)
+add_public_tablegen_target(MLIRLinalgNamedStructuredOpsYamlIncGen)
 
 # Provide a short name for all external dependency that needs to
 # include Linalg in ODS
diff --git a/mlir/lib/Dialect/Linalg/IR/CMakeLists.txt b/mlir/lib/Dialect/Linalg/IR/CMakeLists.txt
index f0ac1899bb02a..65b65014b23cf 100644
--- a/mlir/lib/Dialect/Linalg/IR/CMakeLists.txt
+++ b/mlir/lib/Dialect/Linalg/IR/CMakeLists.txt
@@ -9,6 +9,7 @@ add_mlir_dialect_library(MLIRLinalgDialect
 
   DEPENDS
   MLIRLinalgInterfacesIncGen
+  MLIRLinalgNamedStructuredOpsYamlIncGen
   MLIRLinalgOpsAttributesIncGen
   MLIRLinalgOpsEnumsIncGen
   MLIRLinalgOpsIncGen
diff --git a/mlir/tools/mlir-linalg-ods-gen/CMakeLists.txt b/mlir/tools/mlir-linalg-ods-gen/CMakeLists.txt
index 787a0bb35d7b1..2b2024da6409a 100644
--- a/mlir/tools/mlir-linalg-ods-gen/CMakeLists.txt
+++ b/mlir/tools/mlir-linalg-ods-gen/CMakeLists.txt
@@ -3,26 +3,26 @@ set(LLVM_LINK_COMPONENTS
   Support
   )
 
-# New mlir-linalg-ods-yaml-gen.
-add_mlir_tool(mlir-linalg-ods-yaml-gen
-  mlir-linalg-ods-yaml-gen.cpp
-)
-llvm_update_compile_flags(mlir-linalg-ods-yaml-gen)
-target_link_libraries(mlir-linalg-ods-yaml-gen PRIVATE
+set(LIBS
   MLIRIR
   MLIRSupport
   MLIRParser
-  )
+)
 
-setup_host_tool(mlir-linalg-ods-yaml-gen MLIR_LINALG_ODS_YAML_GEN MLIR_LINALG_ODS_YAML_GEN_EXE MLIR_LINALG_ODS_YAML_GEN_TARGET)
+# New mlir-linalg-ods-yaml-gen.
+add_tablegen(mlir-linalg-ods-yaml-gen MLIR_LINALG_ODS_YAML_GEN
+  DESTINATION "${MLIR_TOOLS_INSTALL_DIR}"
+  EXPORT MLIR
+  SKIP_COMPONENT_LINK
+  mlir-linalg-ods-yaml-gen.cpp
 
-if(NOT ${MLIR_LINALG_ODS_YAML_GEN_EXE} STREQUAL "mlir-linalg-ods-yaml-gen")
-  add_custom_target(mlir-linalg-ods-yaml-gen-host DEPENDS ${MLIR_LINALG_ODS_YAML_GEN_EXE})
+  DEPENDS
+  ${LIBS}
+)
+set_target_properties(mlir-linalg-ods-yaml-gen PROPERTIES FOLDER "Tablegenning")
+target_link_libraries(mlir-linalg-ods-yaml-gen PRIVATE ${LIBS})
 
-  if(NOT LLVM_BUILD_UTILS)
-    set_target_properties(mlir-linalg-ods-yaml-gen PROPERTIES EXCLUDE_FROM_ALL ON)
-  endif()
-endif()
+mlir_check_all_link_libraries(mlir-linalg-ods-yaml-gen)
 
 configure_file(
   update_core_linalg_named_ops.sh.in

MikeHolman · 2023-12-18T19:39:42Z

Can someone please take a look when you have a chance?

Also tagging @stellaraccident who originally authored this.

stellaraccident

Oh thank you - this is a good catch. There seems to be a lot of special categorization I've found for "tablegen" throughout the codebase, and I've often thought that it was a bit of a misnomer that covered "code generator needed for the build that happens to always be a tablegen tool".

MikeHolman · 2024-01-02T19:35:45Z

@stellaraccident I don't have push access, would you be able to help merge it for me?

stellaraccident · 2024-01-02T20:18:39Z

@stellaraccident I don't have push access, would you be able to help merge it for me?

Done. Sorry, new year and vacation intervened.

sylvestre · 2024-01-03T13:09:52Z

@MikeHolman it caused
#76813
could you please revert it or fix it quickly?

MikeHolman · 2024-01-03T17:36:43Z

Looking into this now. Will revert if I can't fix quickly.

…ld (#75093)" This reverts commit 9191ac0. Breaks build on following buildbot: https://lab.llvm.org/buildbot/#/builders/177/builds/27432

…ild (llvm#75093)" This reverts commit a24c581.

doru1004 · 2024-01-03T22:05:37Z

@MikeHolman your original commit was just reverted because it was breaking things. I noticed breakages on my end when building the compiler.

I am seeing the same error reported above on my end.

Do you have a fix?

MikeHolman · 2024-01-03T22:15:41Z

#76843 should fix this.
In particular:
6af1846

doru1004 · 2024-01-03T22:18:27Z

Thank you for the fix! When I saw the reapply, I thought it was in main. :)

stellaraccident · 2024-01-03T22:29:47Z

Thanks folks. Sorry I couldn't support more - vacation. If this needs more iteration in the next week or two, better to get someone else with hands on keyboard. I do think it is generally the right thing to be doing, but this layer of the build system is old and fiddly. Any details on how it failed can help a lot.

MikeHolman · 2024-01-03T23:18:19Z

@stellaraccident Please, enjoy your vacation! We can get this sorted or it can wait a couple more weeks.

The issue seems to be that LLVM Support library is being linked against twice in shared library builds. I believe the solution is that we don't need to explicitly link against Support here, and I have removed the linkage in #76843.

I think this is because in shared library builds, the llvm_config macro (which is called by add_tablegen via add_llvm_executable) will link mlir-linalg-ods-yaml-gen against libLLVM, which already contains libSupport, and that causes the debug-counter command line option to be registered twice.

joshua-arch1 · 2024-11-21T02:04:25Z

#76843 should fix this. In particular: 6af1846

Does this patch solve the breaking issue? When I both reapply the "Consider mlir-linalg-ods-gen as a tablegen tool in build" patch as well as the "fix linking" patch, I can stilll see "Check for working C compiler: .../bin/clang - broken".

MikeHolman · 2024-11-22T19:25:51Z

@joshua-arch1 I thought it did fix the issue, but it is possible what I repro'd was not the same problem you are seeing.

MikeHolman requested review from dcaballe and nicolasvasilache as code owners December 11, 2023 20:18

llvmbot added cmake Build system in general and CMake in particular mlir:core MLIR Core Infrastructure mlir:linalg mlir labels Dec 11, 2023

Consider mlir-linalg-ods-gen a tablegen tool in build

d93f472

MikeHolman force-pushed the linalg-ods-tablegen branch from 0b52d5b to d93f472 Compare December 13, 2023 17:23

stellaraccident self-requested a review December 19, 2023 16:38

stellaraccident approved these changes Dec 19, 2023

View reviewed changes

stellaraccident merged commit 9191ac0 into llvm:main Jan 2, 2024

omjavaid added a commit that referenced this pull request Jan 3, 2024

Revert "[mlir] Consider mlir-linalg-ods-gen as a tablegen tool in bui…

a24c581

…ld (#75093)" This reverts commit 9191ac0. Breaks build on following buildbot: https://lab.llvm.org/buildbot/#/builders/177/builds/27432

MikeHolman added a commit to MikeHolman/llvm-project that referenced this pull request Jan 3, 2024

Reapply "[mlir] Consider mlir-linalg-ods-gen as a tablegen tool in bu…

5f385a5

…ild (llvm#75093)" This reverts commit a24c581.

llvmbot mentioned this pull request Nov 21, 2024

Question about the mlir-linalg-ods-gen patch #117114

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[mlir] Consider mlir-linalg-ods-gen as a tablegen tool in build #75093

[mlir] Consider mlir-linalg-ods-gen as a tablegen tool in build #75093

Uh oh!

MikeHolman commented Dec 11, 2023

Uh oh!

llvmbot commented Dec 11, 2023 •

edited

Loading

Uh oh!

MikeHolman commented Dec 18, 2023

Uh oh!

stellaraccident left a comment

Uh oh!

MikeHolman commented Jan 2, 2024

Uh oh!

stellaraccident commented Jan 2, 2024

Uh oh!

sylvestre commented Jan 3, 2024

Uh oh!

MikeHolman commented Jan 3, 2024

Uh oh!

doru1004 commented Jan 3, 2024

Uh oh!

MikeHolman commented Jan 3, 2024 •

edited

Loading

Uh oh!

doru1004 commented Jan 3, 2024

Uh oh!

stellaraccident commented Jan 3, 2024

Uh oh!

MikeHolman commented Jan 3, 2024

Uh oh!

joshua-arch1 commented Nov 21, 2024

Uh oh!

MikeHolman commented Nov 22, 2024

Uh oh!

Uh oh!

[mlir] Consider mlir-linalg-ods-gen as a tablegen tool in build #75093

[mlir] Consider mlir-linalg-ods-gen as a tablegen tool in build #75093

Uh oh!

Conversation

MikeHolman commented Dec 11, 2023

Uh oh!

llvmbot commented Dec 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MikeHolman commented Dec 18, 2023

Uh oh!

stellaraccident left a comment

Choose a reason for hiding this comment

Uh oh!

MikeHolman commented Jan 2, 2024

Uh oh!

stellaraccident commented Jan 2, 2024

Uh oh!

sylvestre commented Jan 3, 2024

Uh oh!

MikeHolman commented Jan 3, 2024

Uh oh!

doru1004 commented Jan 3, 2024

Uh oh!

MikeHolman commented Jan 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

doru1004 commented Jan 3, 2024

Uh oh!

stellaraccident commented Jan 3, 2024

Uh oh!

MikeHolman commented Jan 3, 2024

Uh oh!

joshua-arch1 commented Nov 21, 2024

Uh oh!

MikeHolman commented Nov 22, 2024

Uh oh!

Uh oh!

llvmbot commented Dec 11, 2023 •

edited

Loading

MikeHolman commented Jan 3, 2024 •

edited

Loading