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

[Martin Storsjö via Phabricator via llvm-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