Avoid repeating CMake checks across runtimes by unifying names of
variables used for results to leverage CMake caching.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxxabi/cmake/config-ix.cmake | ||
---|---|---|
86 | The newly added checks in llvm-project/runtimes (for -nostdinc++ and -nostdlib++) probably should be aligned with these too. |
While I think that the direction of this change is amazing, I'm less enthusiastic about the naming. I think that it is particularly confusing for the flags to use C and CXX as the prefixes for the flags as we have llvm-libc (aka "c") and libc++ (aka "c++", "cxx"). Using a more verbose name of COMPILER_ or C_COMPILER_, CXX_COMPILER_ would allow the flags to be easily attributed to the compiler or the runtime. However, the C standard flags definitely should be dropped.
compiler-rt/CMakeLists.txt | ||
---|---|---|
460 | Can we rename this? I don't think that C_SUPPORTS_NODEFAULTLIBS_FLAG is ideal - how about COMPILER_SUPPORTS_... or C_COMPILER_...? | |
compiler-rt/cmake/config-ix.cmake | ||
60–61 | Similar, COMPILER_SUPPORTS_ or C_COMPILER_SUPPORTS_.... The flag isn't about the C library | |
61 | I think that we should get rid of this check entirely. Instead, we should use the C_STANDARD and C_STANDARD_REQUIRED properties on the targets. This has been available since CMake 3.1. |
This is exactly the kind of suggestion I was looking for, thanks! I'm fine with either, the only reason why C_COMPILER_/CXX_COMPILER_ might be preferable over simple COMPILER_ is if there we ever wanted to distinguish between flags supported by C and C++ compiler (in particular if there was a flag that was only supported in one mode but not the other). I'm not sure if that's ever going to be the case?
Do you have any thoughts on variable naming for libraries? We seem to have HAVE_LIBPTHREAD, LIBCXX_HAS_PTHREAD_LIB and COMPILER_RT_HAS_LIBPTHREAD.
I agree with @compnerd , I think this is pure awesomeness. Also agree with the naming. My take would be:
CXX_COMPILER_SUPPORTS_<flag> C_COMPILER_SUPPORTS_<flag> PLATFORM_SUPPORTS_LIB_<library>
Requesting changes so it shows up in my review queue properly, but this LGTM in essence. Also we'll want to get a green CI.
Any updates to this one? Now that the standalone builds of runtime projects have been removed, this would be rather nice to have, to reap easy benefits from building all projects in one cmake invocation. (I guess this one doesn't involve any really complicated cmake trickery either, so if you'd don't have time for it, maybe I should consider to pick it up.)
Commandeering this patch, as it'd be nice to have it landed, and should be doable to get consensus on.
Rebased to current git main. I split out the parts concerning -std=c11 in compiler-rt, as those can be handled differently, separately from this.
I also split out rewriting of the variable name for the -ffreestanding flag in compiler-rt, as that test doesn't seem to be shared with any of the other runtimes.
So far, I think the scope of the patch was to rename all of the variable names within libcxx/libcxxabi/libunwind, and the ones in compiler-rt that overlaps any of them, but there's lots of more variables in compiler-rt that could be renamed consistently, even if it doesn't add any extra cache sharing across projects.
I guess the naming of variables was indeed the only main issue to settle here. The currently proposed naming in this patch originally, e.g. CXX_SUPPORTS_..., does match what LLVM uses in add_flag_if_supported here: https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/HandleLLVMOptions.cmake#L260-L265
So in this current form, this patch can also get cache hits between tests from llvm/cmake/modules and the rest of the runtimes, while if we settle on a different naming scheme like CXX_COMPILER_SUPPORTS_... we won't get matches between tests in those different areas. (Not sure if there are any right now, but there could be.) And expanding this patch to rename the naming style of LLVM's cmake modules would grow the scope of this patch quite significantly.
Or what's @beanz 's opinion on it?
Thanks for taking over, I was far too busy over the last few weeks, but I should have more time for the next few weeks if you need help.
Right, that's why I chose the naming scheme in the original patch, to avoid having to rename variables in llvm/cmake/modules and reduce the churn.
I think it's doable, but if we decide that's the direction we want to take, then I'd do it as a separate change .
My naming suggestion wasn't meant to stall this effort. I still have a slight preference for mentioning _COMPILER_ in the name of these variables, but I don't care strongly enough to block this. I also think we'll want to lift these checks even further into runtimes/ to simplify the code, but that's certainly a separate effort.
I am fine with this as-is so I'll lift the hard "requires-changes" on the runtimes, but please wait to see whether others who had commented are fine too before shipping.
Yes, the next step I had planned after this is to lift https://github.com/llvm/llvm-project/blob/df209b8038f3803621e96799f5c43e9145448e10/libcxx/cmake/Modules/HandleLibcxxFlags.cmake to runtimes/cmake/Modules, and then replace https://github.com/llvm/llvm-project/blob/df209b8038f3803621e96799f5c43e9145448e10/libcxxabi/cmake/Modules/HandleLibcxxabiFlags.cmake and https://github.com/llvm/llvm-project/blob/df209b8038f3803621e96799f5c43e9145448e10/libunwind/cmake/Modules/HandleLibunwindFlags.cmake (and eventually also https://github.com/llvm/llvm-project/blob/main/compiler-rt/cmake/Modules/CompilerRTUtils.cmake but that's going to take more effort). Once this change lands, those changes should be effectively no-ops.
Ping @compnerd - can you look at the reasoning and arguments above? While your suggested naming would be clearer, it would be different from the rest of what's used in llvm/cmake/*, and I'd prefer not to start refactoring the naming of everything in the full llvm cmake universe for the sake of this patch - I'm just enabling reuse within some of the runtimes.
Sure, the argument of the cache hits is definitely reasonable - I'm not a fan of the naming, but can understand the desire to not rename all the variables across all the projects.
Can we rename this? I don't think that C_SUPPORTS_NODEFAULTLIBS_FLAG is ideal - how about COMPILER_SUPPORTS_... or C_COMPILER_...?