Page MenuHomePhabricator

[runtimes] [CMake] Unify variable names
ClosedPublic

Authored by mstorsjo on Sep 17 2021, 2:38 PM.

Details

Reviewers
MaskRay
compnerd
ldionne
phosek
beanz
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Commits
rGb3df14b6c987: [runtimes] [CMake] Unify variable names
Summary

Avoid repeating CMake checks across runtimes by unifying names of
variables used for results to leverage CMake caching.

Diff Detail

Event Timeline

phosek created this revision.Sep 17 2021, 2:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 17 2021, 2:38 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
phosek requested review of this revision.Sep 17 2021, 2:38 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptSep 17 2021, 2:38 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a subscriber: Restricted Project. · View Herald Transcript
MaskRay accepted this revision.Sep 17 2021, 4:05 PM
MaskRay added a subscriber: MaskRay.

Looks great to me. Looks like CXX_SUPPORTS_ is already used.

mstorsjo added inline comments.
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.

compnerd requested changes to this revision.Sep 20 2021, 3:20 PM
compnerd added a subscriber: compnerd.

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 revision now requires changes to proceed.Sep 20 2021, 3:20 PM

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.

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.

ldionne requested changes to this revision.Sep 23 2021, 9:31 AM
ldionne added a subscriber: ldionne.

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.

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

Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2022, 1:56 AM
mstorsjo commandeered this revision.Mar 24 2022, 5:28 AM
mstorsjo added a reviewer: phosek.

Commandeering this patch, as it'd be nice to have it landed, and should be doable to get consensus on.

mstorsjo updated this revision to Diff 417892.Mar 24 2022, 5:31 AM

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.

mstorsjo added a subscriber: beanz.

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.

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 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?

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.

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.

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?

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 .

ldionne accepted this revision.Mar 24 2022, 3:20 PM

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.

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.

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.

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.

Ping @compnerd

compnerd accepted this revision.Apr 23 2022, 9:05 AM

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.

This revision is now accepted and ready to land.Apr 23 2022, 9:05 AM
mstorsjo updated this revision to Diff 424745.Apr 23 2022, 1:10 PM

Rebased, rerunning CI - will push later if everything still passes.

mstorsjo retitled this revision from [WIP][CMake] Unify variable names to [runtimes] [CMake] Unify variable names.Apr 23 2022, 1:10 PM
This revision was landed with ongoing or failed builds.Apr 24 2022, 3:19 AM
This revision was automatically updated to reflect the committed changes.