[PATCH] D99621: [CMake][Compiler-rt] Make it possible to configure standalone compiler-rt without `LLVMConfig.cmake`.

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: compiler-rt/cmake/Modules/CompilerRTMockLLVMCMakeConfig.cmake:36
+    execute_process(
+      COMMAND "${CMAKE_C_COMPILER}" -print-target-triple
+      RESULT_VARIABLE HAD_ERROR
----------------
delcypher wrote:
> mstorsjo wrote:
> > Clang supports `-dumpmachine` too - is there any functional difference between the two?
> Not that I know of. I wrote support for clang first and then I thought I should probably add gcc support just incase that's what you were using to build compiler-rt. I can merge the branches to use the same flag if that's what you prefer.
I guess that'd be a bit more straightforward to handle both gcc and clang with the same, maybe with a comment that clang also supports `-print-target-triple`.


================
Comment at: compiler-rt/cmake/Modules/CompilerRTMockLLVMCMakeConfig.cmake:47
+  else()
+    message(FATAL_ERROR
+      "Fetching target triple from compiler \"${CMAKE_C_COMPILER_ID}\" "
----------------
delcypher wrote:
> mstorsjo wrote:
> > If building with e.g. `-DCMAKE_C_COMPILER_TARGET=<triple> -DCOMPILER_RT_DEFAULT_TARGET_ONLY=TRUE`, which sets `TARGET_TRIPLE` somehow (I don't know offhand where/how), I guess that we shouldn't try to do this detection here, but just go with whatever was provided that way?
> > 
> > It also seems like it's possible to build by just setting `TARGET_TRIPLE` only (without setting `COMPILER_RT_DEFAULT_TARGET_ONLY`) - that kinda works but has odd/unexpected effects for me (by building both i386 and x86_64 at the same time, when I've been used to only building one arch at a time).
> > If building with e.g. -DCMAKE_C_COMPILER_TARGET=<triple> -DCOMPILER_RT_DEFAULT_TARGET_ONLY=TRUE, which sets TARGET_TRIPLE somehow (I don't know offhand where/how),
> 
> Are you sure about this? I can't reproduce when running like this (without this patch applied)
> 
> ```
> cmake  -GNinja -DLLVM_CONFIG_PATH="" -DCMAKE_C_COMPILER_TARGET=x86_64-apple-darwin20.3.0  -DCOMPILER_RT_DEFAULT_TARGET_ONLY=TRUE -DCOMPILER_RT_INCLUDE_TESTS=ON /path/compiler-rt
> ```
> 
> Where are you looking for the value of `TARGET_TRIPLE` to show up? One place you can look for it in your build directory is `unittests/lit.common.unit.configured`. I see
> 
> ```
> config.target_triple = ""
> ```
> 
> Note don't look at `test/lit.common.configured`. That has `COMPILER_RT_DEFAULT_TARGET_TRIPLE` expanded and not `TARGET_TRIPLE` into the `"target_triple"` lit config, which is extremely confusing.
> 
> > I guess that we shouldn't try to do this detection here, but just go with whatever was provided that way?
> 
> We could... I guess we're interacting with this existing code...
> 
> ```lang=cmake
> macro(construct_compiler_rt_default_triple)
>   if(COMPILER_RT_DEFAULT_TARGET_ONLY)
>     if(DEFINED COMPILER_RT_DEFAULT_TARGET_TRIPLE)
>       message(FATAL_ERROR "COMPILER_RT_DEFAULT_TARGET_TRIPLE isn't supported when building for default target only")
>     endif()
>     set(COMPILER_RT_DEFAULT_TARGET_TRIPLE ${CMAKE_C_COMPILER_TARGET})
>   else()
>     set(COMPILER_RT_DEFAULT_TARGET_TRIPLE ${TARGET_TRIPLE} CACHE STRING
>           "Default triple for which compiler-rt runtimes will be built.")
>   endif()
>   ...
> ```
> 
> So I guess `compiler_rt_mock_llvm_cmake_config_set_target_triple` could have an early exit... to use `CMAKE_C_COMPILER_TARGET` if `COMPILER_RT_DEFAULT_TARGET_ONLY` is set. However, there's no point doing that if `TARGET_TRIPLE` is actually getting set elsewhere because the results will just overwritten.
> 
> Can you confirm that `TARGET_TRIPLE` is actually being set using the method I outlined above?
> 
> > It also seems like it's possible to build by just setting TARGET_TRIPLE only (without setting COMPILER_RT_DEFAULT_TARGET_ONLY) - that kinda works but has odd/unexpected effects for me (by building both i386 and x86_64 at the same time, when I've been used to only building one arch at a time).
> 
> I'd said this is both expected and unexpected. I'd expect that because compiler-rt normally builds for multiple architectures (at least it does in the configurations I normally use) and so the name of `COMPILER_RT_DEFAULT_TARGET_ONLY` does what it suggests, it builds only one target. So if you don't use the option you get builds for multiple architectures.
> 
> It's also a little unexpected because having `TARGET_TRIPLE` be a single architecture might suggest that's the only thing compiler-rt should compile for. But as you've seen that's not what happens.
> 
> The fact that this variable exists at all and is used is feels like a build system design mistake but fixing that isn't really in the scope of this patch.
Sorry - I wasn't sure it was setting `TARGET_TRIPLE` - I just kinda assumed that was the internal variable that ends up set.

If I configure that way, I actually get the default `TARGET_TRIPLE` of the llvm build (provided that llvm-config can be found), not the one I set in `CMAKE_C_COMPILER_TARGET`.

So, sorry for the confusion.

(But I see that when building via llvm/runtimes, it sets both `COMPILER_RT_DEFAULT_TARGET_ONLY`, and I think, `TARGET_TRIPLE`, but not `CMAKE_C_COMPILER_TARGET`, but when I try doing that myself, it errors out due to the list of architectures ending up empty, " -- Compiler-RT supported architectures:")

Doing anything about the case of building multiple architectures at once is out of scope indeed. (Do you happen to know how to request what set of architectures to build for, can I request building for eg i386, x86_64, armv7 and aarch64 at once, of is it just all x86 variants at once?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99621