[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

[Martin Storsjö via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGd3da9067d143: [CMake] Allow setting the location of host 
tools with LLVM_NATIVE_TOOL_DIR (authored by mstorsjo).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131052/new/

https://reviews.llvm.org/D131052

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/pseudo/include/CMakeLists.txt
  llvm/CMakeLists.txt
  llvm/cmake/modules/AddLLVM.cmake
  llvm/cmake/modules/TableGen.cmake
  llvm/tools/llvm-config/CMakeLists.txt
  mlir/tools/mlir-linalg-ods-gen/CMakeLists.txt

Index: mlir/tools/mlir-linalg-ods-gen/CMakeLists.txt
===
--- mlir/tools/mlir-linalg-ods-gen/CMakeLists.txt
+++ mlir/tools/mlir-linalg-ods-gen/CMakeLists.txt
@@ -14,23 +14,13 @@
   MLIRParser
   )
 
-set(MLIR_LINALG_ODS_YAML_GEN mlir-linalg-ods-yaml-gen CACHE
-  STRING "Native mlir-linalg-ods-yaml-gen executable. Saves building one when cross-compiling.")
+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)
 
-set(MLIR_LINALG_ODS_YAML_GEN_EXE ${MLIR_LINALG_ODS_YAML_GEN} PARENT_SCOPE)
-set(MLIR_LINALG_ODS_YAML_GEN_TARGET mlir-linalg-ods-yaml-gen PARENT_SCOPE)
+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})
 
-if(LLVM_USE_HOST_TOOLS)
-  if (${MLIR_LINALG_ODS_YAML_GEN} STREQUAL "mlir-linalg-ods-yaml-gen")
-build_native_tool(mlir-linalg-ods-yaml-gen MLIR_LINALG_ODS_YAML_GEN_EXE DEPENDS mlir-linalg-ods-yaml-gen)
-set(MLIR_LINALG_ODS_YAML_GEN_EXE ${MLIR_LINALG_ODS_YAML_GEN_EXE} PARENT_SCOPE)
-
-add_custom_target(mlir-linalg-ods-yaml-gen-host DEPENDS ${MLIR_LINALG_ODS_YAML_GEN_EXE})
-set(MLIR_LINALG_ODS_YAML_GEN_TARGET mlir-linalg-ods-yaml-gen-host DEPENDS PARENT_SCOPE)
-
-if(NOT LLVM_BUILD_UTILS)
-  set_target_properties(mlir-linalg-ods-yaml-gen PROPERTIES EXCLUDE_FROM_ALL ON)
-endif()
+  if(NOT LLVM_BUILD_UTILS)
+set_target_properties(mlir-linalg-ods-yaml-gen PROPERTIES EXCLUDE_FROM_ALL ON)
   endif()
 endif()
 
Index: llvm/tools/llvm-config/CMakeLists.txt
===
--- llvm/tools/llvm-config/CMakeLists.txt
+++ llvm/tools/llvm-config/CMakeLists.txt
@@ -91,10 +91,18 @@
 # Add the dependency on the generation step.
 add_file_dependencies(${CMAKE_CURRENT_SOURCE_DIR}/llvm-config.cpp ${BUILDVARIABLES_OBJPATH})
 
-if(CMAKE_CROSSCOMPILING AND NOT LLVM_CONFIG_PATH)
-  build_native_tool(llvm-config LLVM_CONFIG_PATH)
-  set(LLVM_CONFIG_PATH "${LLVM_CONFIG_PATH}" CACHE STRING "")
+if(CMAKE_CROSSCOMPILING)
+  if (LLVM_NATIVE_TOOL_DIR AND NOT LLVM_CONFIG_PATH)
+if (EXISTS "${LLVM_NATIVE_TOOL_DIR}/llvm-config${LLVM_HOST_EXECUTABLE_SUFFIX}")
+  set(LLVM_CONFIG_PATH "${LLVM_NATIVE_TOOL_DIR}/llvm-config${LLVM_HOST_EXECUTABLE_SUFFIX}")
+endif()
+  endif()
 
-  add_custom_target(NativeLLVMConfig DEPENDS ${LLVM_CONFIG_PATH})
-  add_dependencies(llvm-config NativeLLVMConfig)
+  if (NOT LLVM_CONFIG_PATH)
+build_native_tool(llvm-config LLVM_CONFIG_PATH)
+set(LLVM_CONFIG_PATH "${LLVM_CONFIG_PATH}" CACHE STRING "")
+
+add_custom_target(NativeLLVMConfig DEPENDS ${LLVM_CONFIG_PATH})
+add_dependencies(llvm-config NativeLLVMConfig)
+  endif()
 endif()
Index: llvm/cmake/modules/TableGen.cmake
===
--- llvm/cmake/modules/TableGen.cmake
+++ llvm/cmake/modules/TableGen.cmake
@@ -156,7 +156,13 @@
 ${ADD_TABLEGEN_UNPARSED_ARGUMENTS})
   set(LLVM_LINK_COMPONENTS ${${target}_OLD_LLVM_LINK_COMPONENTS})
 
-  set(${project}_TABLEGEN "${target}" CACHE
+  set(${project}_TABLEGEN_DEFAULT "${target}")
+  if (LLVM_NATIVE_TOOL_DIR)
+if (EXISTS "${LLVM_NATIVE_TOOL_DIR}/${target}${LLVM_HOST_EXECUTABLE_SUFFIX}")
+  set(${project}_TABLEGEN_DEFAULT "${LLVM_NATIVE_TOOL_DIR}/${target}${LLVM_HOST_EXECUTABLE_SUFFIX}")
+endif()
+  endif()
+  set(${project}_TABLEGEN "${${project}_TABLEGEN_DEFAULT}" CACHE
   STRING "Native TableGen executable. Saves building one when cross-compiling.")
 
   # Effective tblgen executable to be used:
Index: llvm/cmake/modules/AddLLVM.cmake
===
--- llvm/cmake/modules/AddLLVM.cmake
+++ llvm/cmake/modules/AddLLVM.cmake
@@ -2397,3 +2397,31 @@
 endif()
   endif()
 endfunction()
+
+function(setup_host_tool tool_name setting_name exe_var_name target_var_name)
+  set(${setting_name}_DEFAULT "${tool_name}")
+
+  if(LLVM_USE_HOST_TOOLS AND LLVM_NATIVE_TOOL_DIR)
+if(EXISTS "${LLVM_NATIVE_TOOL_DIR}/${tool_name}${LLVM_HOST_EXECUTABLE_SUFFIX}")
+  set(${setting_name}_DEFAULT 

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

[Chris Bieneman via Phabricator via cfe-commits]

beanz accepted this revision.
beanz added a comment.
This revision is now accepted and ready to land.

LGTM!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131052/new/

https://reviews.llvm.org/D131052

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D131052#4052563 , @mstorsjo wrote:

> Switched the macro to a function and changed the variables to cache 
> variables, to allow setting them in the grandparent scope without being a 
> macro.

@beanz - Does this seems ok to you now?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131052/new/

https://reviews.llvm.org/D131052

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo updated this revision to Diff 489098.
mstorsjo added a comment.

Switched the macro to a function and changed the variables to cache variables, 
to allow setting them in the grandparent scope without being a macro.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131052/new/

https://reviews.llvm.org/D131052

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/pseudo/include/CMakeLists.txt
  llvm/CMakeLists.txt
  llvm/cmake/modules/AddLLVM.cmake
  llvm/cmake/modules/TableGen.cmake
  llvm/tools/llvm-config/CMakeLists.txt
  mlir/tools/mlir-linalg-ods-gen/CMakeLists.txt

Index: mlir/tools/mlir-linalg-ods-gen/CMakeLists.txt
===
--- mlir/tools/mlir-linalg-ods-gen/CMakeLists.txt
+++ mlir/tools/mlir-linalg-ods-gen/CMakeLists.txt
@@ -14,23 +14,13 @@
   MLIRParser
   )
 
-set(MLIR_LINALG_ODS_YAML_GEN mlir-linalg-ods-yaml-gen CACHE
-  STRING "Native mlir-linalg-ods-yaml-gen executable. Saves building one when cross-compiling.")
+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)
 
-set(MLIR_LINALG_ODS_YAML_GEN_EXE ${MLIR_LINALG_ODS_YAML_GEN} PARENT_SCOPE)
-set(MLIR_LINALG_ODS_YAML_GEN_TARGET mlir-linalg-ods-yaml-gen PARENT_SCOPE)
+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})
 
-if(LLVM_USE_HOST_TOOLS)
-  if (${MLIR_LINALG_ODS_YAML_GEN} STREQUAL "mlir-linalg-ods-yaml-gen")
-build_native_tool(mlir-linalg-ods-yaml-gen MLIR_LINALG_ODS_YAML_GEN_EXE DEPENDS mlir-linalg-ods-yaml-gen)
-set(MLIR_LINALG_ODS_YAML_GEN_EXE ${MLIR_LINALG_ODS_YAML_GEN_EXE} PARENT_SCOPE)
-
-add_custom_target(mlir-linalg-ods-yaml-gen-host DEPENDS ${MLIR_LINALG_ODS_YAML_GEN_EXE})
-set(MLIR_LINALG_ODS_YAML_GEN_TARGET mlir-linalg-ods-yaml-gen-host DEPENDS PARENT_SCOPE)
-
-if(NOT LLVM_BUILD_UTILS)
-  set_target_properties(mlir-linalg-ods-yaml-gen PROPERTIES EXCLUDE_FROM_ALL ON)
-endif()
+  if(NOT LLVM_BUILD_UTILS)
+set_target_properties(mlir-linalg-ods-yaml-gen PROPERTIES EXCLUDE_FROM_ALL ON)
   endif()
 endif()
 
Index: llvm/tools/llvm-config/CMakeLists.txt
===
--- llvm/tools/llvm-config/CMakeLists.txt
+++ llvm/tools/llvm-config/CMakeLists.txt
@@ -91,10 +91,18 @@
 # Add the dependency on the generation step.
 add_file_dependencies(${CMAKE_CURRENT_SOURCE_DIR}/llvm-config.cpp ${BUILDVARIABLES_OBJPATH})
 
-if(CMAKE_CROSSCOMPILING AND NOT LLVM_CONFIG_PATH)
-  build_native_tool(llvm-config LLVM_CONFIG_PATH)
-  set(LLVM_CONFIG_PATH "${LLVM_CONFIG_PATH}" CACHE STRING "")
+if(CMAKE_CROSSCOMPILING)
+  if (LLVM_NATIVE_TOOL_DIR AND NOT LLVM_CONFIG_PATH)
+if (EXISTS "${LLVM_NATIVE_TOOL_DIR}/llvm-config${LLVM_HOST_EXECUTABLE_SUFFIX}")
+  set(LLVM_CONFIG_PATH "${LLVM_NATIVE_TOOL_DIR}/llvm-config${LLVM_HOST_EXECUTABLE_SUFFIX}")
+endif()
+  endif()
 
-  add_custom_target(NativeLLVMConfig DEPENDS ${LLVM_CONFIG_PATH})
-  add_dependencies(llvm-config NativeLLVMConfig)
+  if (NOT LLVM_CONFIG_PATH)
+build_native_tool(llvm-config LLVM_CONFIG_PATH)
+set(LLVM_CONFIG_PATH "${LLVM_CONFIG_PATH}" CACHE STRING "")
+
+add_custom_target(NativeLLVMConfig DEPENDS ${LLVM_CONFIG_PATH})
+add_dependencies(llvm-config NativeLLVMConfig)
+  endif()
 endif()
Index: llvm/cmake/modules/TableGen.cmake
===
--- llvm/cmake/modules/TableGen.cmake
+++ llvm/cmake/modules/TableGen.cmake
@@ -156,7 +156,13 @@
 ${ADD_TABLEGEN_UNPARSED_ARGUMENTS})
   set(LLVM_LINK_COMPONENTS ${${target}_OLD_LLVM_LINK_COMPONENTS})
 
-  set(${project}_TABLEGEN "${target}" CACHE
+  set(${project}_TABLEGEN_DEFAULT "${target}")
+  if (LLVM_NATIVE_TOOL_DIR)
+if (EXISTS "${LLVM_NATIVE_TOOL_DIR}/${target}${LLVM_HOST_EXECUTABLE_SUFFIX}")
+  set(${project}_TABLEGEN_DEFAULT "${LLVM_NATIVE_TOOL_DIR}/${target}${LLVM_HOST_EXECUTABLE_SUFFIX}")
+endif()
+  endif()
+  set(${project}_TABLEGEN "${${project}_TABLEGEN_DEFAULT}" CACHE
   STRING "Native TableGen executable. Saves building one when cross-compiling.")
 
   # Effective tblgen executable to be used:
Index: llvm/cmake/modules/AddLLVM.cmake
===
--- llvm/cmake/modules/AddLLVM.cmake
+++ llvm/cmake/modules/AddLLVM.cmake
@@ -2397,3 +2397,31 @@
 endif()
   endif()
 endfunction()
+
+function(setup_host_tool tool_name setting_name exe_var_name target_var_name)
+  set(${setting_name}_DEFAULT "${tool_name}")
+
+  if(LLVM_USE_HOST_TOOLS AND LLVM_NATIVE_TOOL_DIR)
+if(EXISTS "${LLVM_NATIVE_TOOL_DIR}/${tool_name}${LLVM_HOST_EXECUTABLE_SUFFIX}")
+  set(${setting_name}_DEFAULT "${LLVM_NATIVE_TOOL_DIR}/${tool_name}${LLVM_HOST_EXECUTABLE_SUFFIX}")
+endif()
+  endif()
+
+  

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D131052#4051948 , @beanz wrote:

> The convention that `find_program` uses is to cache the variables, which 
> causes them to be defined at global scope.

Right, I guess that could work too. It would be less of a pure refactoring 
though, but still probably wouldn't need further changes in the call sites. I 
can try to make that change.

> That also avoids needing to recompute filesystem lookups in incremental 
> builds, which is desirable.

Yeah, that's always desireable.

(In this case, the filesystem lookups are in the setup of the 
`_DEFAULT` variables though, so that wouldn't be cached here unless 
we make that variable a cache variable too - but that would only affect users 
who set the `LLVM_NATIVE_TOOL_DIR` variable.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131052/new/

https://reviews.llvm.org/D131052

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

The convention that `find_program` uses is to cache the variables, which causes 
them to be defined at global scope. That also avoids needing to recompute 
filesystem lookups in incremental builds, which is desirable.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131052/new/

https://reviews.llvm.org/D131052

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added inline comments.



Comment at: llvm/cmake/modules/AddLLVM.cmake:2401
+
+macro(setup_host_tool tool_name setting_name exe_var_name target_var_name)
+  cmake_parse_arguments(ARG "" "SCOPE" "" ${ARGN})

beanz wrote:
> Please make this a `function` instead of a `macro`. In general CMake `macros` 
> expand in ways that are unintuitive which can be a maintenance burden for 
> people coming in and modifying the code.
> 
> Also a bit nity, but maybe a name like `find_host_program` would be more 
> consistent naming. I suspect you could even simplify this implementation by 
> using CMake's `find_program` 
> (https://cmake.org/cmake/help/latest/command/find_program.html)
Re `function` vs `macro` - in `mlir-linalg-ods-gen`, we need to set the 
variable in the parent scope (i.e. the CMakeLists.txt of the surrounding 
directory); afaik with a `function` we can set the variable in the scope of the 
caller, but not in the caller's parent - or am I missing something?

Re naming, I think `find_host_program` as name feels a bit unintuitive for the 
regular (non-cross compiling) case; we've set up a target for building a tool 
and we want to execute it at build time - plus doing the boilerplate setup of 
building a separate host version of it when cross compiling. The fact that we 
may want to use a preexisting binary, if the user has pointed us to it, is the 
exception in the uncommon case here, so I wouldn't want to name the high level 
structure based on that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131052/new/

https://reviews.llvm.org/D131052

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

[Chris Bieneman via Phabricator via cfe-commits]

beanz added inline comments.



Comment at: llvm/cmake/modules/AddLLVM.cmake:2401
+
+macro(setup_host_tool tool_name setting_name exe_var_name target_var_name)
+  cmake_parse_arguments(ARG "" "SCOPE" "" ${ARGN})

Please make this a `function` instead of a `macro`. In general CMake `macros` 
expand in ways that are unintuitive which can be a maintenance burden for 
people coming in and modifying the code.

Also a bit nity, but maybe a name like `find_host_program` would be more 
consistent naming. I suspect you could even simplify this implementation by 
using CMake's `find_program` 
(https://cmake.org/cmake/help/latest/command/find_program.html)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131052/new/

https://reviews.llvm.org/D131052

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo updated this revision to Diff 488982.
mstorsjo added a comment.
Herald added subscribers: Moerafaat, zero9178, bzcheeseman, sdasgup3, 
wenzhicui, wrengr, cota, teijeong, rdzhabarov, tatianashp, msifontes, jurahul, 
Kayjukh, grosul1, Joonsoo, stephenneuendorffer, liufengdb, aartbik, mgester, 
arpith-jacob, nicolasvasilache, antiagainst, shauheen, rriddle, mehdi_amini.
Herald added a reviewer: njames93.
Herald added a project: MLIR.

Factorized setup of building a separate host version of code generation tools 
into a macro, shared across existing users.

There are some minor nuance differences to how tool generation happened before, 
but it should probably not matter (and this is probably an improvement):

- Only look at the user setting variable (for pointing to a preexisting 
external tool executable) if LLVM_USE_HOST_TOOLS is set; if LLVM_USE_HOST_TOOLS 
isn't set, ignore the variable. (mlir-linalg-ods-gen already did it this way, 
the clang-tools-extra tools didn't.)
- Add a dependency on the target version of the executable, when building a 
native one - this makes sure to regenereate the native one when there are 
changes to the sources for that tool (according to other cmake comments in the 
code; the clang-tools-extra tools didn't do this before).

If wanted, I could split this up into two patches; one for refactoring the call 
sites and one for adding the new variable LLVM_NATIVE_TOOL_DIR - but I'm 
posting them as one single patch here for now, to hopefully get it reviewed 
quicker.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131052/new/

https://reviews.llvm.org/D131052

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/pseudo/include/CMakeLists.txt
  llvm/CMakeLists.txt
  llvm/cmake/modules/AddLLVM.cmake
  llvm/cmake/modules/TableGen.cmake
  llvm/tools/llvm-config/CMakeLists.txt
  mlir/tools/mlir-linalg-ods-gen/CMakeLists.txt

Index: mlir/tools/mlir-linalg-ods-gen/CMakeLists.txt
===
--- mlir/tools/mlir-linalg-ods-gen/CMakeLists.txt
+++ mlir/tools/mlir-linalg-ods-gen/CMakeLists.txt
@@ -14,23 +14,13 @@
   MLIRParser
   )
 
-set(MLIR_LINALG_ODS_YAML_GEN mlir-linalg-ods-yaml-gen CACHE
-  STRING "Native mlir-linalg-ods-yaml-gen executable. Saves building one when cross-compiling.")
+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 SCOPE PARENT_SCOPE)
 
-set(MLIR_LINALG_ODS_YAML_GEN_EXE ${MLIR_LINALG_ODS_YAML_GEN} PARENT_SCOPE)
-set(MLIR_LINALG_ODS_YAML_GEN_TARGET mlir-linalg-ods-yaml-gen PARENT_SCOPE)
+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})
 
-if(LLVM_USE_HOST_TOOLS)
-  if (${MLIR_LINALG_ODS_YAML_GEN} STREQUAL "mlir-linalg-ods-yaml-gen")
-build_native_tool(mlir-linalg-ods-yaml-gen MLIR_LINALG_ODS_YAML_GEN_EXE DEPENDS mlir-linalg-ods-yaml-gen)
-set(MLIR_LINALG_ODS_YAML_GEN_EXE ${MLIR_LINALG_ODS_YAML_GEN_EXE} PARENT_SCOPE)
-
-add_custom_target(mlir-linalg-ods-yaml-gen-host DEPENDS ${MLIR_LINALG_ODS_YAML_GEN_EXE})
-set(MLIR_LINALG_ODS_YAML_GEN_TARGET mlir-linalg-ods-yaml-gen-host DEPENDS PARENT_SCOPE)
-
-if(NOT LLVM_BUILD_UTILS)
-  set_target_properties(mlir-linalg-ods-yaml-gen PROPERTIES EXCLUDE_FROM_ALL ON)
-endif()
+  if(NOT LLVM_BUILD_UTILS)
+set_target_properties(mlir-linalg-ods-yaml-gen PROPERTIES EXCLUDE_FROM_ALL ON)
   endif()
 endif()
 
Index: llvm/tools/llvm-config/CMakeLists.txt
===
--- llvm/tools/llvm-config/CMakeLists.txt
+++ llvm/tools/llvm-config/CMakeLists.txt
@@ -91,10 +91,18 @@
 # Add the dependency on the generation step.
 add_file_dependencies(${CMAKE_CURRENT_SOURCE_DIR}/llvm-config.cpp ${BUILDVARIABLES_OBJPATH})
 
-if(CMAKE_CROSSCOMPILING AND NOT LLVM_CONFIG_PATH)
-  build_native_tool(llvm-config LLVM_CONFIG_PATH)
-  set(LLVM_CONFIG_PATH "${LLVM_CONFIG_PATH}" CACHE STRING "")
+if(CMAKE_CROSSCOMPILING)
+  if (LLVM_NATIVE_TOOL_DIR AND NOT LLVM_CONFIG_PATH)
+if (EXISTS "${LLVM_NATIVE_TOOL_DIR}/llvm-config${LLVM_HOST_EXECUTABLE_SUFFIX}")
+  set(LLVM_CONFIG_PATH "${LLVM_NATIVE_TOOL_DIR}/llvm-config${LLVM_HOST_EXECUTABLE_SUFFIX}")
+endif()
+  endif()
 
-  add_custom_target(NativeLLVMConfig DEPENDS ${LLVM_CONFIG_PATH})
-  add_dependencies(llvm-config NativeLLVMConfig)
+  if (NOT LLVM_CONFIG_PATH)
+build_native_tool(llvm-config LLVM_CONFIG_PATH)
+set(LLVM_CONFIG_PATH "${LLVM_CONFIG_PATH}" CACHE STRING "")
+
+add_custom_target(NativeLLVMConfig DEPENDS ${LLVM_CONFIG_PATH})
+add_dependencies(llvm-config NativeLLVMConfig)
+  endif()
 endif()
Index: llvm/cmake/modules/TableGen.cmake
===
--- llvm/cmake/modules/TableGen.cmake
+++ 

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

Yeah, this makes sense to me if we factor out the commonalities. I agree that 
it's nicer to have a single option vs. having to specify every single native 
tool individually.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131052/new/

https://reviews.llvm.org/D131052

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

TL;DR: Thanks for explanation, LG if we move the common logic into a macro.

In D131052#3734986 , @mstorsjo wrote:

> In D131052#3731894 , @sammccall 
> wrote:
>
>> FWIW I have no idea: in principle this makes sense, but I don't use such a 
>> configuration and don't have a clear idea of what people who do use it want.
>
> Thanks for having a look and discussing the matter! FWIW, my current cross 
> compilation script does something like this: 
> https://github.com/mstorsjo/llvm-mingw/blob/master/build-llvm.sh#L176-L195 
> All of those lines could be changed into setting this one option.
>
> When there's more native tools added, I want to keep this in sync to avoid 
> needless recompilation of those tools - which is kinda burdensome in the 
> current setup - while with this option, we'd only need to set one option and 
> be done with it. (As long as everything else works, if there's a new tool 
> that I haven't got hooked up, it "only" makes things slower to build.)

Thanks, if this option is useful for you then I think it's probably useful for 
others too.
(Just want to make sure we're not adding features for no-one)

>> It also adds significant CMake complexity: e.g. clang-pseudo-gen now has 30 
>> lines just to support the non-default "native tools" configuration, and this 
>> is duplicated for each tool. Maybe this could be made cheaper by sharing 
>> more of this logic in a separate place, but we should probably only add it 
>> at all if this really is helping someone significantly.
>>
>> (Lines of CMake logic are extremely expensive to maintain IME: tooling and 
>> diagnostics are poor, there are no tests, there are too many configurations 
>> to reason about, interacting components are not documented, the language is 
>> inherently confusing and many of us that maintain it have only a rudimentary 
>> understanding)
>
> This is a very good point indeed.
>
> Most of the boilerplate for setting up clang-pseudo-gen and 
> clang-tidy-confusable-chars-gen is identical, so those could probably be 
> merged into a macro (hiding all the cross/native complexity entirely from the 
> business logic, just like for tablegen) - but the complexity is still there. 
> (It would end up in at least three cases; tablegen, llvm-config and in a 
> generic-buildtime-tool-macro.)

I think the generic tool macro would be valuable even if it can't be used for 
tablegen (though it's not obvious at first glance why not).
In practice if we copy the logic in two places it's extremely likely to get 
copied in a third and/or modified by someone who doesn't really understand it 
at some point. A macro would make this less likely, the macro name itself is 
documentation, it increases the benefit/cost of having a comment.

> On the topic of "really is helping someone significantly" - if we wouldn't 
> have the burden of existing users using the existing options (for setting the 
> path to each tool individually), picking this new option is a nobrainer - 
> doing the same thing but in a much less annoying way. But since we have 
> existing users with the existing options (which would need to be kept for 
> some time transitionally), it's much less clear cut whether it's a 
> "significant" improvement.

Agree this is probably a much nicer structure. (Only concern: is it likely that 
someone wants to use LLVM tools from out-of-tree but build clang tools in-tree?)
If it's plausible to remove the existing options, it probably makes sense to do 
it sooner rather than later - it'll break people the same amount either way.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131052/new/

https://reviews.llvm.org/D131052

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D131052#3731894 , @sammccall wrote:

> FWIW I have no idea: in principle this makes sense, but I don't use such a 
> configuration and don't have a clear idea of what people who do use it want.

Thanks for having a look and discussing the matter! FWIW, my current cross 
compilation script does something like this: 
https://github.com/mstorsjo/llvm-mingw/blob/master/build-llvm.sh#L176-L195 All 
of those lines could be changed into setting this one option.

When there's more native tools added, I want to keep this in sync to avoid 
needless recompilation of those tools - which is kinda burdensome in the 
current setup - while with this option, we'd only need to set one option and be 
done with it. (As long as everything else works, if there's a new tool that I 
haven't got hooked up, it "only" makes things slower to build.)

> It also adds significant CMake complexity: e.g. clang-pseudo-gen now has 30 
> lines just to support the non-default "native tools" configuration, and this 
> is duplicated for each tool. Maybe this could be made cheaper by sharing more 
> of this logic in a separate place, but we should probably only add it at all 
> if this really is helping someone significantly.
>
> (Lines of CMake logic are extremely expensive to maintain IME: tooling and 
> diagnostics are poor, there are no tests, there are too many configurations 
> to reason about, interacting components are not documented, the language is 
> inherently confusing and many of us that maintain it have only a rudimentary 
> understanding)

This is a very good point indeed.

Most of the boilerplate for setting up clang-pseudo-gen and 
clang-tidy-confusable-chars-gen is identical, so those could probably be merged 
into a macro (hiding all the cross/native complexity entirely from the business 
logic, just like for tablegen) - but the complexity is still there. (It would 
end up in at least three cases; tablegen, llvm-config and in a 
generic-buildtime-tool-macro.)

On the topic of "really is helping someone significantly" - if we wouldn't have 
the burden of existing users using the existing options (for setting the path 
to each tool individually), picking this new option is a nobrainer - doing the 
same thing but in a much less annoying way. But since we have existing users 
with the existing options (which would need to be kept for some time 
transitionally), it's much less clear cut whether it's a "significant" 
improvement.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131052/new/

https://reviews.llvm.org/D131052

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

FWIW I have no idea: in principle this makes sense, but I don't use such a 
configuration and don't have a clear idea of what people who do use it want.

It also adds significant CMake complexity: e.g. clang-pseudo-gen now has 30 
lines just to support the non-default "native tools" configuration, and this is 
duplicated for each tool. Maybe this could be made cheaper by sharing more of 
this logic in a separate place, but we should probably only add it at all if 
this really is helping someone significantly.

(Lines of CMake logic are extremely expensive to maintain IME: tooling and 
diagnostics are poor, there are no tests, there are too many configurations to 
reason about, interacting components are not documented, the language is 
inherently confusing and many of us that maintain it have only a rudimentary 
understanding)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131052/new/

https://reviews.llvm.org/D131052

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

Ping - is there any interest in this - a single flag for pointing towards 
existing prebuilt native executables instead of having to name every one 
(llvm-tblgen, clang-tblgen, lldb-tblgen, clang-pseudo-gen, 
clang-tidy-confusable-chars-gen) individually?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131052/new/

https://reviews.llvm.org/D131052

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo created this revision.
mstorsjo added reviewers: sammccall, tstellar, smeenai, beanz.
Herald added subscribers: carlosgalvezp, mgorny.
Herald added a project: All.
mstorsjo requested review of this revision.
Herald added projects: LLVM, clang-tools-extra.
Herald added a subscriber: cfe-commits.

This avoids having to specify the location of all individual tools.
In current builds, one may want to specify LLVM_TABLEGEN, CLANG_TABLEGEN,
LLDB_TABLEGEN, LLVM_CONFIG_PATH, CLANG_PSEUDO_GEN and
CLANG_TIDY_CONFUSABLE_CHARS_GEN; specifying just the base directory
containing all of them is much more convenient.

Note that HOST_EXECUTABLE_SUFFIX is different from CMAKE_EXECUTABLE_SUFFIX
(the suffix used for the cross compiled binaries). There's an upstream
request for CMake to provide such a variable, e.g.
CMAKE_HOST_EXECUTABLE_SUFFIX, in
https://gitlab.kitware.com/cmake/cmake/-/issues/17553, but it hasn't been
implemented yet.

This is a bit of an RFC, we could of course factorize setting of
HOST_EXECUTABLE_SUFFIX to a helper function/macro, and possibly
factorize the whole setup (including checking LLVM_USE_HOST_TOOLS and
calling build_native_tools) of clang-pseudo-gen and
clang-tidy-confusable-chars-gen, as it does end up as a fair bit of
boilerplate.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131052

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/pseudo/include/CMakeLists.txt
  llvm/CMakeLists.txt
  llvm/cmake/modules/TableGen.cmake
  llvm/tools/llvm-config/CMakeLists.txt

Index: llvm/tools/llvm-config/CMakeLists.txt
===
--- llvm/tools/llvm-config/CMakeLists.txt
+++ llvm/tools/llvm-config/CMakeLists.txt
@@ -82,10 +82,23 @@
 # Add the dependency on the generation step.
 add_file_dependencies(${CMAKE_CURRENT_SOURCE_DIR}/llvm-config.cpp ${BUILDVARIABLES_OBJPATH})
 
-if(CMAKE_CROSSCOMPILING AND NOT LLVM_CONFIG_PATH)
-  build_native_tool(llvm-config LLVM_CONFIG_PATH)
-  set(LLVM_CONFIG_PATH "${LLVM_CONFIG_PATH}" CACHE STRING "")
+if(CMAKE_CROSSCOMPILING)
+  if (LLVM_NATIVE_TOOL_DIR AND NOT LLVM_CONFIG_PATH)
+if (CMAKE_HOST_WIN32)
+  set(HOST_EXECUTABLE_SUFFIX ".exe")
+else()
+  set(HOST_EXECUTABLE_SUFFIX "")
+endif()
+if (EXISTS "${LLVM_NATIVE_TOOL_DIR}/llvm-config${HOST_EXECUTABLE_SUFFIX}")
+  set(LLVM_CONFIG_PATH "${LLVM_NATIVE_TOOL_DIR}/llvm-config${HOST_EXECUTABLE_SUFFIX}")
+endif()
+  endif()
+
+  if (NOT LLVM_CONFIG_PATH)
+build_native_tool(llvm-config LLVM_CONFIG_PATH)
+set(LLVM_CONFIG_PATH "${LLVM_CONFIG_PATH}" CACHE STRING "")
 
-  add_custom_target(NativeLLVMConfig DEPENDS ${LLVM_CONFIG_PATH})
-  add_dependencies(llvm-config NativeLLVMConfig)
+add_custom_target(NativeLLVMConfig DEPENDS ${LLVM_CONFIG_PATH})
+add_dependencies(llvm-config NativeLLVMConfig)
+  endif()
 endif()
Index: llvm/cmake/modules/TableGen.cmake
===
--- llvm/cmake/modules/TableGen.cmake
+++ llvm/cmake/modules/TableGen.cmake
@@ -152,7 +152,18 @@
   add_llvm_executable(${target} DISABLE_LLVM_LINK_LLVM_DYLIB ${ARGN})
   set(LLVM_LINK_COMPONENTS ${${target}_OLD_LLVM_LINK_COMPONENTS})
 
-  set(${project}_TABLEGEN "${target}" CACHE
+  set(${project}_TABLEGEN_DEFAULT "${target}")
+  if (LLVM_NATIVE_TOOL_DIR)
+if (CMAKE_HOST_WIN32)
+  set(HOST_EXECUTABLE_SUFFIX ".exe")
+else()
+  set(HOST_EXECUTABLE_SUFFIX "")
+endif()
+if (EXISTS "${LLVM_NATIVE_TOOL_DIR}/${target}${HOST_EXECUTABLE_SUFFIX}")
+  set(${project}_TABLEGEN_DEFAULT "${LLVM_NATIVE_TOOL_DIR}/${target}${HOST_EXECUTABLE_SUFFIX}")
+endif()
+  endif()
+  set(${project}_TABLEGEN "${${project}_TABLEGEN_DEFAULT}" CACHE
   STRING "Native TableGen executable. Saves building one when cross-compiling.")
 
   # Effective tblgen executable to be used:
Index: llvm/CMakeLists.txt
===
--- llvm/CMakeLists.txt
+++ llvm/CMakeLists.txt
@@ -598,6 +598,7 @@
 if( WIN32 AND NOT CYGWIN )
   set(LLVM_LIT_TOOLS_DIR "" CACHE PATH "Path to GnuWin32 tools")
 endif()
+set(LLVM_NATIVE_TOOL_DIR "" CACHE PATH "Path to a directory containing prebuilt matching native tools (such as llvm-tblgen)")
 
 set(LLVM_INTEGRATED_CRT_ALLOC "" CACHE PATH "Replace the Windows CRT allocator with any of {rpmalloc|mimalloc|snmalloc}. Only works with /MT enabled.")
 if(LLVM_INTEGRATED_CRT_ALLOC)
Index: clang-tools-extra/pseudo/include/CMakeLists.txt
===
--- clang-tools-extra/pseudo/include/CMakeLists.txt
+++ clang-tools-extra/pseudo/include/CMakeLists.txt
@@ -1,7 +1,20 @@
 # The cxx.bnf grammar file
 set(cxx_bnf ${CMAKE_CURRENT_SOURCE_DIR}/../lib/cxx/cxx.bnf)
 
-set(CLANG_PSEUDO_GEN "clang-pseudo-gen" CACHE
+set(CLANG_PSEUDO_GEN_DEFAULT "clang-pseudo-gen")
+
+if (LLVM_NATIVE_TOOL_DIR)
+  if (CMAKE_HOST_WIN32)
+set(HOST_EXECUTABLE_SUFFIX