llvm dev - Sep 2020 - Unifying CMake variable names used in checks across subprojects

> On Sep 22, 2020, at 15:28, Eric Christopher <echristo at gmail.com>
wrote:
> 
> From the "not largely affected" camp:
> 
>  - the churn doesn't feel that major for HAS_ and ...
>  - the uniformity feels nice
> 
> and in general feels nice and in pursuit of the longer term goals here. 
> 
> -eric
> 
> On Tue, Sep 22, 2020 at 11:58 AM Petr Hosek via llvm-dev <llvm-dev at
lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
> We've been using the runtimes build for a while now and we're very
happy with it. However, with an increasing number of targets, it can be fairly
slow and I have noticed that we now spend more time in CMake than in Ninja.
There are various ways we could improve things like eliminating unnecessary
checks.
> 
> When running checks like check_c_compiler_flag, check_cxx_compiler_flag or
check_library_exists, CMake caches the resulting variable and doesn't run
the check again. The problem is that in LLVM, each subproject uses different
variable names for results of these checks. For example, most subprojects check
if pthread is available and store the result in:
> 
> COMPILER_RT_HAS_LIBPTHREAD (compiler-rt)
> LIBCXX_HAS_PTHREAD_LIB (libc++)
> LIBCXXABI_HAS_PTHREAD_LIB (libc++abi)
> LIBUNWIND_HAS_PTHREAD_LIB (libunwind)
> HAVE_LIBPTHREAD (llvm)
> 
> This means that even though this check would ideally be performed just once
(per target) and reused everywhere, it's performed 5 times. The same is true
for most flags and library checks.
> 
> I think that this is really unnecessary and could be easily improved by
unifying CMake variable names used in checks across subprojects to benefit from
caching.
> 
> I've looked at naming conventions used across all subprojects and
I'm proposing the following:
> 
> C_SUPPORTS_${mangled_name}_FLAG for check_c_compiler_flag
> CXX_SUPPORTS_${mangled_name}_FLAG for check_cxx_compiler_flag
> HAVE_${mangled_name} for check_library_exists
IMO, these issues are a manifestation of the fact that we basically have (at
least) 4 times the same overall logic, once for each runtime project:
compiler-rt, libunwind, libcxxabi, libcxx.

At the end of the day, what we're trying to achieve is link against the
right system libraries when building the various runtimes. Would it make sense
to instead bundle together the logic of searching for these libraries and adding
the right compiler flags? We could use interface targets to achieve that. IOW,
from libc++'s CMake, I'd love to just be able to write:

    target_link_libraries(cxx PUBLIC runtimes-system-libraries)

This would add -lpthread, -lgcc -lgcc_s, -lSystem or whatever else is needed on
the system. I think this approach would provide more build system simplification
and be more robust in the long term than relying on a naming convention to
achieve sharing. What do you think?

Louis

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200922/ff337c40/attachment.html>

Using more interface libraries is definitely the right direction and a
modern way to use CMake. I'm not sure if we can get to a single interface
target since different runtimes have different requirements. I was assuming
that we would have one interface target per dependency and use the existing
CMake support where it already exists, for example use the FindThreads
module to handle pthreads.

I mostly want to ensure that we're not letting the perfect be the enemy of
the good. We've been talking about more major CMake refactorings for some
time, but we haven't made much progress so far, partially because nobody
has a clear idea what the end state is going to look like. I think that
this proposal can be implemented pretty quickly (it's mostly just a bunch
of grep & sed) and while it's not the end state we want, it's a
stepping
stone which would make an immediate impact on users. After this change, we
can start introducing interface targets and later factoring those out once
we make more progress on setting up the common CMake infrastructure. Does
that make sense?

On Tue, Sep 22, 2020 at 12:42 PM Louis Dionne <ldionne at apple.com>
wrote:
> On Sep 22, 2020, at 15:28, Eric Christopher <echristo at gmail.com>
wrote:
>
> From the "not largely affected" camp:
>
>  - the churn doesn't feel that major for HAS_ and ...
>  - the uniformity feels nice
>
> and in general feels nice and in pursuit of the longer term goals here.
>
> -eric
>
> On Tue, Sep 22, 2020 at 11:58 AM Petr Hosek via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> We've been using the runtimes build for a while now and we're
very happy
>> with it. However, with an increasing number of targets, it can be
fairly
>> slow and I have noticed that we now spend more time in CMake than in
Ninja.
>> There are various ways we could improve things like eliminating
unnecessary
>> checks.
>>
>> When running checks like check_c_compiler_flag, check_cxx_compiler_flag
>> or check_library_exists, CMake caches the resulting variable and
doesn't
>> run the check again. The problem is that in LLVM, each subproject uses
>> different variable names for results of these checks. For example, most
>> subprojects check if pthread is available and store the result in:
>>
>> COMPILER_RT_HAS_LIBPTHREAD (compiler-rt)
>> LIBCXX_HAS_PTHREAD_LIB (libc++)
>> LIBCXXABI_HAS_PTHREAD_LIB (libc++abi)
>> LIBUNWIND_HAS_PTHREAD_LIB (libunwind)
>> HAVE_LIBPTHREAD (llvm)
>>
>> This means that even though this check would ideally be performed just
>> once (per target) and reused everywhere, it's performed 5 times.
The same
>> is true for most flags and library checks.
>>
>> I think that this is really unnecessary and could be easily improved by
>> unifying CMake variable names used in checks across subprojects to
benefit
>> from caching.
>>
>> I've looked at naming conventions used across all subprojects and
I'm
>> proposing the following:
>>
>> C_SUPPORTS_${mangled_name}_FLAG for check_c_compiler_flag
>> CXX_SUPPORTS_${mangled_name}_FLAG for check_cxx_compiler_flag
>> HAVE_${mangled_name} for check_library_exists
>>
>
> IMO, these issues are a manifestation of the fact that we basically have
> (at least) 4 times the same overall logic, once for each runtime project:
> compiler-rt, libunwind, libcxxabi, libcxx.
>
> At the end of the day, what we're trying to achieve is link against the
> right system libraries when building the various runtimes. Would it make
> sense to instead bundle together the logic of searching for these libraries
> and adding the right compiler flags? We could use interface targets to
> achieve that. IOW, from libc++'s CMake, I'd love to just be able to
write:
>
>     target_link_libraries(cxx PUBLIC runtimes-system-libraries)
>
> This would add -lpthread, -lgcc -lgcc_s, -lSystem or whatever else is
> needed on the system. I think this approach would provide more build system
> simplification and be more robust in the long term than relying on a naming
> convention to achieve sharing. What do you think?
>
> Louis
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200924/08cabc30/attachment-0001.html>

> On Sep 24, 2020, at 23:52, Petr Hosek <phosek at chromium.org> wrote:
> 
> Using more interface libraries is definitely the right direction and a
modern way to use CMake. I'm not sure if we can get to a single interface
target since different runtimes have different requirements. I was assuming that
we would have one interface target per dependency and use the existing CMake
support where it already exists, for example use the FindThreads module to
handle pthreads.
> 
> I mostly want to ensure that we're not letting the perfect be the enemy
of the good. We've been talking about more major CMake refactorings for some
time, but we haven't made much progress so far, partially because nobody has
a clear idea what the end state is going to look like. I think that this
proposal can be implemented pretty quickly (it's mostly just a bunch of grep
& sed) and while it's not the end state we want, it's a stepping
stone which would make an immediate impact on users. After this change, we can
start introducing interface targets and later factoring those out once we make
more progress on setting up the common CMake infrastructure. Does that make
sense?
Sure, I agree. It is indeed difficult to make time for (and justify) improving
to the build system. I'll take a look if you send a patch.

Louis
> 
> On Tue, Sep 22, 2020 at 12:42 PM Louis Dionne <ldionne at apple.com
<mailto:ldionne at apple.com>> wrote:
>> On Sep 22, 2020, at 15:28, Eric Christopher <echristo at gmail.com
<mailto:echristo at gmail.com>> wrote:
>> 
>> From the "not largely affected" camp:
>> 
>>  - the churn doesn't feel that major for HAS_ and ...
>>  - the uniformity feels nice
>> 
>> and in general feels nice and in pursuit of the longer term goals here.
>> 
>> -eric
>> 
>> On Tue, Sep 22, 2020 at 11:58 AM Petr Hosek via llvm-dev <llvm-dev
at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
>> We've been using the runtimes build for a while now and we're
very happy with it. However, with an increasing number of targets, it can be
fairly slow and I have noticed that we now spend more time in CMake than in
Ninja. There are various ways we could improve things like eliminating
unnecessary checks.
>> 
>> When running checks like check_c_compiler_flag, check_cxx_compiler_flag
or check_library_exists, CMake caches the resulting variable and doesn't run
the check again. The problem is that in LLVM, each subproject uses different
variable names for results of these checks. For example, most subprojects check
if pthread is available and store the result in:
>> 
>> COMPILER_RT_HAS_LIBPTHREAD (compiler-rt)
>> LIBCXX_HAS_PTHREAD_LIB (libc++)
>> LIBCXXABI_HAS_PTHREAD_LIB (libc++abi)
>> LIBUNWIND_HAS_PTHREAD_LIB (libunwind)
>> HAVE_LIBPTHREAD (llvm)
>> 
>> This means that even though this check would ideally be performed just
once (per target) and reused everywhere, it's performed 5 times. The same is
true for most flags and library checks.
>> 
>> I think that this is really unnecessary and could be easily improved by
unifying CMake variable names used in checks across subprojects to benefit from
caching.
>> 
>> I've looked at naming conventions used across all subprojects and
I'm proposing the following:
>> 
>> C_SUPPORTS_${mangled_name}_FLAG for check_c_compiler_flag
>> CXX_SUPPORTS_${mangled_name}_FLAG for check_cxx_compiler_flag
>> HAVE_${mangled_name} for check_library_exists
> 
> IMO, these issues are a manifestation of the fact that we basically have
(at least) 4 times the same overall logic, once for each runtime project:
compiler-rt, libunwind, libcxxabi, libcxx.
> 
> At the end of the day, what we're trying to achieve is link against the
right system libraries when building the various runtimes. Would it make sense
to instead bundle together the logic of searching for these libraries and adding
the right compiler flags? We could use interface targets to achieve that. IOW,
from libc++'s CMake, I'd love to just be able to write:
> 
>     target_link_libraries(cxx PUBLIC runtimes-system-libraries)
> 
> This would add -lpthread, -lgcc -lgcc_s, -lSystem or whatever else is
needed on the system. I think this approach would provide more build system
simplification and be more robust in the long term than relying on a naming
convention to achieve sharing. What do you think?
> 
> Louis
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200925/99205e0d/attachment.html>