Fix CMake dependencies on mlir-linalg-ods-yaml-gen #113565
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix a number of dependencies issue to build mlir-linalg-ods-yaml-gen
host binary which make a cross-build using the Make generator fail.
Namely:
- do not use binary path for the custom target created when
LLVM_USE_HOST_TOOLS is true;
- use target name instead of name of variable holding the target name
for add_custom_target and set_target_properties in setup_host_tool();
- force setting of executable and target cache variable which are only
used as global variables;
- remove dependency on target defined in different directory in
add_linalg_ods_yaml_gen() since add_custom_target DEPENDS can only be
used on "files and outputs of custom commands created with
add_custom_command() command calls in the same directory";
- remove unneeded dependency on ${MLIR_LINALG_ODS_YAML_GEN_EXE}, the
target dependency will ensure the binary will be built.
Note that we keep using ${MLIR_LINALG_ODS_YAML_GEN_EXE} in the COMMAND
rather than use ${MLIR_LINALG_ODS_YAML_GEN_TARGET} because when
LLVM_NATIVE_TOOL_DIR is used the latter is an empty string.
Testing-wise, all three codepaths in get_host_tool_path() were tested
with both GNU Make and Ninja generators:
- cross-compiling with LLVM_NATIVE_TOOL_DIR checks the if path;
- cross-compiling without LLVM_NATIVE_TOOL_DIR checks the elseif path;
- native build without LLVM_NATIVE_TOOL_DIR checks the else path.
RoboTux
requested review from
dcaballe,
nicolasvasilache and
rengolin
as code owners
llvmbot
added
cmake
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-linalg Author: Thomas Preud'homme (RoboTux) ChangesFix a number of dependencies issue to build mlir-linalg-ods-yaml-gen
Note that we keep using ${MLIR_LINALG_ODS_YAML_GEN_EXE} in the COMMAND Testing-wise, all three codepaths in get_host_tool_path() were tested
Full diff: https://github.com/llvm/llvm-project/pull/113565.diff 2 Files Affected:
diff --git a/llvm/cmake/modules/AddLLVM.cmake b/llvm/cmake/modules/AddLLVM.cmake
index 9f5e4344b898a7..006dfb6de3a199 100644
--- a/llvm/cmake/modules/AddLLVM.cmake
+++ b/llvm/cmake/modules/AddLLVM.cmake
@@ -2618,13 +2618,16 @@ function(get_host_tool_path tool_name setting_name exe_var_name target_var_name)
set(target_name "")
elseif(LLVM_USE_HOST_TOOLS)
get_native_tool_path(${tool_name} exe_name)
- set(target_name ${exe_name})
+ set(target_name host_${tool_name})
else()
set(exe_name $<TARGET_FILE:${tool_name}>)
set(target_name ${tool_name})
endif()
- set(${exe_var_name} "${exe_name}" CACHE STRING "")
- set(${target_var_name} "${target_name}" CACHE STRING "")
+ # Force setting the cache variable because they are only used for being
+ # global in scope. Using regular variables would require careful audit of the
+ # code with several parent scope set commands.
+ set(${exe_var_name} "${exe_name}" CACHE STRING "" FORCE)
+ set(${target_var_name} "${target_name}" CACHE STRING "" FORCE)
endfunction()
function(setup_host_tool tool_name setting_name exe_var_name target_var_name)
@@ -2632,8 +2635,8 @@ function(setup_host_tool tool_name setting_name exe_var_name target_var_name)
# Set up a native tool build if necessary
if(LLVM_USE_HOST_TOOLS AND NOT ${setting_name})
build_native_tool(${tool_name} exe_name DEPENDS ${tool_name})
- add_custom_target(${target_var_name} DEPENDS ${exe_name})
+ add_custom_target(${${target_var_name}} DEPENDS ${exe_name})
get_subproject_title(subproject_title)
- set_target_properties(${target_var_name} PROPERTIES FOLDER "${subproject_title}/Native")
+ set_target_properties(${${target_var_name}} PROPERTIES FOLDER "${subproject_title}/Native")
endif()
endfunction()
diff --git a/mlir/include/mlir/Dialect/Linalg/IR/CMakeLists.txt b/mlir/include/mlir/Dialect/Linalg/IR/CMakeLists.txt
index 289c0e4bbdaf68..71214b4404c550 100644
--- a/mlir/include/mlir/Dialect/Linalg/IR/CMakeLists.txt
+++ b/mlir/include/mlir/Dialect/Linalg/IR/CMakeLists.txt
@@ -15,13 +15,10 @@ function(add_linalg_ods_yaml_gen yaml_ast_file output_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})
set_target_properties(MLIR${output_file}YamlIncGen PROPERTIES FOLDER "MLIR/Tablegenning")
list(APPEND LLVM_TARGET_DEPENDS ${GEN_ODS_FILE})
|
|
This is a respin of #112224. Note: to be able to test it I had to apply the following other PRs: |
RoboTux
requested review from
mstorsjo,
stellaraccident,
igorkudrin and
omjavaid
|
buildkite/github-pull-requests CI failure is due to a failed test so seems unrelated (build was successful):
|
|
Ping? |
1 similar comment
|
Ping? |
|
Ping? |
stellaraccident
approved these changes
Nov 27, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/171/builds/11261 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/71/builds/11512 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/64/builds/1569 Here is the relevant piece of the build log for the reference |
Thank you! I can't blame you, clearly nobody is comfortable with that part of the build system. It seems this version did the trick, 3rd time's the charm as they say. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix a number of dependencies issue to build mlir-linalg-ods-yaml-gen
host binary which make a cross-build using the Make generator fail.
Namely:
LLVM_USE_HOST_TOOLS is true;
for add_custom_target and set_target_properties in setup_host_tool();
used as global variables;
add_linalg_ods_yaml_gen() since add_custom_target DEPENDS can only be
used on "files and outputs of custom commands created with
add_custom_command() command calls in the same directory";
target dependency will ensure the binary will be built.
Note that we keep using ${MLIR_LINALG_ODS_YAML_GEN_EXE} in the COMMAND
rather than use ${MLIR_LINALG_ODS_YAML_GEN_TARGET} because when
LLVM_NATIVE_TOOL_DIR is used the latter is an empty string.
Testing-wise, all three codepaths in get_host_tool_path() were tested
with both GNU Make and Ninja generators: