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

[mstorsjo]

Summary

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.

[mstorsjo]

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?

[sammccall]

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)

[mstorsjo]

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.

[sammccall]

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.

[smeenai]

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.

[mstorsjo]

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.

[beanz]

llvm/cmake/modules/AddLLVM.cmake

2401 ↗

(On Diff #488982)

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)

[mstorsjo]

llvm/cmake/modules/AddLLVM.cmake

2401 ↗

(On Diff #488982)

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.

[beanz]

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.

[mstorsjo]

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 <TOOLNAME>_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.)

[mstorsjo]

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.

[mstorsjo]

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?

[beanz]

LGTM!

clang-tools-extra/clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
-set(CLANG_TIDY_CONFUSABLE_CHARS_GEN "clang-tidy-confusable-chars-gen" CACHE
+set(CLANG_TIDY_CONFUSABLE_CHARS_GEN_DEFAULT "clang-tidy-confusable-chars-gen")
+
+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}/clang-tidy-confusable-chars-gen${HOST_EXECUTABLE_SUFFIX}")
+    set(CLANG_TIDY_CONFUSABLE_CHARS_GEN_DEFAULT "${LLVM_NATIVE_TOOL_DIR}/clang-tidy-confusable-chars-gen${HOST_EXECUTABLE_SUFFIX}")
+  endif()
+endif()
+
+
+set(CLANG_TIDY_CONFUSABLE_CHARS_GEN "${CLANG_TIDY_CONFUSABLE_CHARS_GEN_DEFAULT}" CACHE
   STRING "Host clang-tidy-confusable-chars-gen executable. Saves building if cross-compiling.")
 
 if(NOT CLANG_TIDY_CONFUSABLE_CHARS_GEN STREQUAL "clang-tidy-confusable-chars-gen")
   set(make_confusable_table ${CLANG_TIDY_CONFUSABLE_CHARS_GEN})
   set(make_confusable_table_target ${CLANG_TIDY_CONFUSABLE_CHARS_GEN})
 elseif(LLVM_USE_HOST_TOOLS)
   build_native_tool(clang-tidy-confusable-chars-gen make_confusable_table)
   set(make_confusable_table_target "${make_confusable_table}")

clang-tools-extra/pseudo/include/CMakeLists.txt

 # 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 ".exe")
+  else()
+    set(HOST_EXECUTABLE_SUFFIX "")
+  endif()
+  if (EXISTS "${LLVM_NATIVE_TOOL_DIR}/clang-pseudo-gen${HOST_EXECUTABLE_SUFFIX}")
+    set(CLANG_PSEUDO_GEN_DEFAULT "${LLVM_NATIVE_TOOL_DIR}/clang-pseudo-gen${HOST_EXECUTABLE_SUFFIX}")
+  endif()
+endif()
+
+set(CLANG_PSEUDO_GEN "${CLANG_PSEUDO_GEN_DEFAULT}" CACHE
   STRING "Host clang-pseudo-gen executable. Saves building if cross-compiling.")
 
 if(NOT CLANG_PSEUDO_GEN STREQUAL "clang-pseudo-gen")
   set(pseudo_gen ${CLANG_PSEUDO_GEN})
   set(pseudo_gen_target ${CLANG_PSEUDO_GEN})
 elseif(LLVM_USE_HOST_TOOLS)
   # The NATIVE executable *must* depend on the current target, otherwise the
   # native one won't get rebuilt when the pseudo-gen sources change.

llvm/CMakeLists.txt

if (MSVC_IDE OR XCODE)
   set(LIT_ARGS_DEFAULT "${LIT_ARGS_DEFAULT} --no-progress-bar")
 endif()
 set(LLVM_LIT_ARGS "${LIT_ARGS_DEFAULT}" CACHE STRING "Default options for lit")
 
 # On Win32 hosts, provide an option to specify the path to the GnuWin32 tools.
 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)
   if(NOT WIN32)
     message(FATAL_ERROR "LLVM_INTEGRATED_CRT_ALLOC is only supported on Windows.")
   endif()
   if(LLVM_USE_SANITIZER)
     message(FATAL_ERROR "LLVM_INTEGRATED_CRT_ALLOC cannot be used along with LLVM_USE_SANITIZER!")

llvm/cmake/modules/TableGen.cmake

macro(add_tablegen target project)
   if(NOT CMAKE_GENERATOR MATCHES "Ninja" AND NOT XCODE)
     # FIXME: It leaks to user, callee of add_tablegen.
     set(LLVM_ENABLE_OBJLIB ON)
   endif()
 
   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:
   set(${project}_TABLEGEN_EXE ${${project}_TABLEGEN} PARENT_SCOPE)
   set(${project}_TABLEGEN_TARGET ${${project}_TABLEGEN} PARENT_SCOPE)
 
   if(LLVM_USE_HOST_TOOLS)
     if( ${${project}_TABLEGEN} STREQUAL "${target}" )

llvm/tools/llvm-config/CMakeLists.txt

if(LLVM_ENABLE_MODULES)
   target_compile_options(llvm-config PUBLIC
     "-fmodules-ignore-macro=CMAKE_CFG_INTDIR"
     )
 endif()
 
 # 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)
+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)
   endif()
+endif()