[PATCH] D64837: [cmake] Convert the NATIVE llvm build process to be project agnostic

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added a comment.

In D64837#1589983 <https://reviews.llvm.org/D64837#1589983>, @lanza wrote:

> In D64837#1588826 <https://reviews.llvm.org/D64837#1588826>, @smeenai wrote:
>
> > This just seems like it's replacing all instances of LLVM with `${CMAKE_PROJECT_NAME}`?
> >
> > I think it'd be cleaner and more explicit to pass an additional project name parameter to `llvm_create_cross_target` instead of implicitly using `${CMAKE_PROJECT_NAME}`. There's some prior art for that, e.g. in https://reviews.llvm.org/diffusion/L/browse/llvm/trunk/cmake/modules/AddLLVM.cmake;366290$1009?as=source&blame=off
>
>
> That's what I did originally. However, the users of `build_natve_tool` wouldn't have the proper context to know what to use. It's only non-llvm specific usage is within `add_tablegen` which would then have to learn how to decide what `project_name` variable to pass in. e.g. if `lldb` is part of `ENABLE_PROJECTS` then `add_tablegen` should call `llvm_native_build(LLVM NATIVE ...)` else `llvm_native_build(LLDB NATIVE ...)`. So we'd have to propagate that to all users of `add_tablegen` where we'd have to figure out whether the build is standalone or not via checking `CMAKE_PROJECT_NAME` anyways. e.g.
>
>    if (${CMAKE_PROJECT_NAME} MATCHES lldb)
>       add_tablegen(lldb lldb lldb-tblgen)
>   else()
>       add_tablegen(llvm lldb lldb-tblgen)
>   endif()
>   
>
> I'm not convinced that adding the granularity to choose your `llvm_create_cross_target` project is worth propagating the configuration outwards given that there are no users that don't satisfy `CMAKE_PROJECT_NAME == project_name`.
>
> An alternative would be to have some global variable `llvm_cross_targets` list that `build_native_tool` iterates over. But this is making an already clumsy solution more clumsy.


I missed the `build_native_tool` thing. In that case, I would have been fine with sticking to `${CMAKE_PROJECT_NAME}` inside `llvm_create_cross_target` as well, but it looks like you've already changed everything to use a project name variable :)



================
Comment at: cmake/modules/CrossCompile.cmake:58
         -DCMAKE_MAKE_PROGRAM="${CMAKE_MAKE_PROGRAM}"
         ${CROSS_TOOLCHAIN_FLAGS_${target_name}} ${CMAKE_SOURCE_DIR}
         -DLLVM_TARGET_IS_CROSSCOMPILE_HOST=TRUE
----------------
beanz wrote:
> smeenai wrote:
> > You'll wanna change this one to read from a project-specific cache variable too. You'll also wanna preserve backwards compatibility for the people using the old spelling.
> I think you probably want to make `CROSS_TOOLCHAIN_FLAGS_${target_name}` generic, not project specific, and only add a project-specific variant if it is needed.
> 
> It is entirely possible to use the cross-compile code to generate multiple different cross-projects for the same target where you do want the same target flags, because those flags relate to the target not the project you're building.
Makes sense. I can see there being a need for project-specific variables as well.

@lanza you should definitely keep the original `${CROSS_TOOLCHAIN_FLAGS_${target_name}}` spelling here. You may also want to add the `${CROSS_TOOLCHAIN_FLAGS_${project_name}_${target_name}}` spelling if e.g. there's some LLDB-specific configurations you'll want to pass? (You could just use the generic `${CROSS_TOOLCHAIN_FLAGS_${target_name}}` for project-specific configs as well; it's slightly ugly, but as long as the configs don't cause issues in other projects it should be fine.)


================
Comment at: cmake/modules/CrossCompile.cmake:27
+  # project specific version of the flags up above
+  set(CROSS_TOOLCHAIN_FLAGS_${project_name} ${target_name} ""
+    CACHE STRING "Toolchain configuration for ${Pproject_name}_${target_name}")
----------------
Are you missing an underscore here? Should the variable be `CROSS_TOOLCHAIN_FLAGS_${project_name}_${target_name}`?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64837