This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement P0226R1 (Mathematical Special Functions)
Needs RevisionPublic

Authored by philnik on Jan 28 2023, 5:17 AM.

Details

Reviewers
ldionne
Mordante
var-const
jdoerfert
tonic
lattner
Group Reviewers
Restricted Project

Diff Detail

Event Timeline

philnik created this revision.Jan 28 2023, 5:17 AM
philnik requested review of this revision.Jan 28 2023, 5:17 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante added a subscriber: tonic.

Thanks for working on this. It's quite a large patch (and a lot of imported code), before looking closer at it, I want to make sure we can distribute libc++ with Boost code. @tonic can you approve this patch if the license is acceptable?

libcxx/src/third-party/LICENSE_1_0.txt
1

@tonic Are we allowed to ship code in libc++ using the Boost license?

lattner requested changes to this revision.Feb 8 2023, 5:09 AM
lattner added a subscriber: lattner.

I'm sorry, but accepting this patch will be very difficult for the project. I have several technical concerns with this patch:

  1. This is a very large block of code which does not follow LLVM conventions and has a lot of boost support functionality that seems redundant with stuff elsewhere in LLVM.
  2. This seems to overlap a lot with the llvm libc work, particularly for the simple numeric operations.
  3. This appears to add code with new dependencies, e.g. gmpfrxx? and has internal abstractions that aren't necessary for libcxx

Furthermore, the use of the boost license is a big issue. While I am not a lawyer, it appears that it could be "compatible" with the LLVM license, but this is not enough.

There are two major issues here:

  1. Different licenses like this add burden to people adopting/using LLVM to vet the code. We'd need a supermajority of the entire LLVM community to approve this before doing this.
  1. Worse yet, it prevents refactoring of the code across boundaries. We want libcxx to be able to evolve freely over time, e.g. when new standards come out. Taking this would mean that we have a split world where some code is in third_party land and some is in libcxx itself, and these future changes would be split across them. Also, the license would prevent moving code form thirdparty to libcxx directly, adversely affecting the architecture of libcxx.

I understand that this is a bunch of fiddly code, I'd recommend reaching out to John Maddock and other contributors. Maybe there is a new path where they are willing to relicense their work and integrate it more piece meal into libcxx, following the conventions of the project.

This revision now requires changes to proceed.Feb 8 2023, 5:09 AM

I'm sorry, but accepting this patch will be very difficult for the project. I have several technical concerns with this patch:

  1. This is a very large block of code which does not follow LLVM conventions and has a lot of boost support functionality that seems redundant with stuff elsewhere in LLVM.
  2. This seems to overlap a lot with the llvm libc work, particularly for the simple numeric operations.
  3. This appears to add code with new dependencies, e.g. gmpfrxx? and has internal abstractions that aren't necessary for libcxx

I have to respectfully disagree with those technical concerns. Setting aside licensing issues, it shouldn't be a problem to use a third-party library to implement complex functionality -- in fact we already have precedent for that: we use Ryu and MSVC's code to implement std::from_chars and we use GoogleBenchmark for our benchmarks. Some things in the Standard Library are so complex that we literally have to rely on a third-party implementation if we are to ever ship it -- we just don't have the resources to do it ourselves. Furthermore, I don't think anything else inside LLVM implements those mathematical special functions, and the C Standard Library doesn't have those, so I don't think it will make sense for LLVM's libc to provide them in a relatively near future.

The way we've done that without compromising libc++'s integrity is by making sure that the third-party code stays an implementation detail of the built library (shared or static), so that users don't end up including that third-party code when they include libc++. And by hiding it behind an ABI boundary that way, we are free to swap the underlying implementation for something else in the future at any time. Modulo a few details, this patch is basically the way I had always envisioned integrating the math special functions into libc++.

Furthermore, the use of the boost license is a big issue. While I am not a lawyer, it appears that it could be "compatible" with the LLVM license, but this is not enough.

I think this is the main issue at play. I do think it is compatible, however the plan has always been to ask the Boost folks to relicense it under the LLVM license, and I think we should definitely do that.

So assuming this were under the LLVM license, I'd be quite happy with taking this patch (after review and a couple changes). @lattner In light of what I said above, are you OK with this plan given that we get everything licensed under the LLVM license?

I'm not familiar with the std::from_chars example, but GoogleBenchmarks is not part of the "shipped" libc++ product, it is an internal implementation detail.

The licensing is definitely the big issue. If that is resolved we can explore whether it makes sense architecturally to have something like this as part of third party. I'm personally very concerned about that (for reasons described upthread) but I'm not an active contributor to the subproject. We should widen the discussion to more people to explore the tradeoffs to see if people think it is significant.

I'm not familiar with the std::from_chars example, but GoogleBenchmarks is not part of the "shipped" libc++ product, it is an internal implementation detail.

The licensing is definitely the big issue. If that is resolved we can explore whether it makes sense architecturally to have something like this as part of third party. I'm personally very concerned about that (for reasons described upthread) but I'm not an active contributor to the subproject. We should widen the discussion to more people to explore the tradeoffs to see if people think it is significant.

That sounds reasonable to me. In that case, I’ll get the ball rolling with the Boost folks on relicensing, and if that goes somewhere, I will post a RFC on Discourse explaining the proposed plan so that folks can chime in. Thanks!

I'm sorry, but accepting this patch will be very difficult for the project. I have several technical concerns with this patch:

  1. This is a very large block of code which does not follow LLVM conventions and has a lot of boost support functionality that seems redundant with stuff elsewhere in LLVM.
  2. This seems to overlap a lot with the llvm libc work, particularly for the simple numeric operations.
  3. This appears to add code with new dependencies, e.g. gmpfrxx? and has internal abstractions that aren't necessary for libcxx

I have to respectfully disagree with those technical concerns. Setting aside licensing issues, it shouldn't be a problem to use a third-party library to implement complex functionality -- in fact we already have precedent for that: we use Ryu and MSVC's code to implement std::from_chars ...

Microsoft provided std::to_chars which is available under multiple licenses. One of these licenses is LLVM license as used by the MSVC STL library.

The way we've done that without compromising libc++'s integrity is by making sure that the third-party code stays an implementation detail of the built library (shared or static), so that users don't end up including that third-party code when they include libc++. And by hiding it behind an ABI boundary that way, we are free to swap the underlying implementation for something else in the future at any time. Modulo a few details, this patch is basically the way I had always envisioned integrating the math special functions into libc++.

Furthermore, the use of the boost license is a big issue. While I am not a lawyer, it appears that it could be "compatible" with the LLVM license, but this is not enough.

I think this is the main issue at play. I do think it is compatible, however the plan has always been to ask the Boost folks to relicense it under the LLVM license, and I think we should definitely do that.

I think that would be great.

FWIW, Microsoft's STL has ingested the boost implementation for the special functions for close to 2 years already (before that it needed to be installed by the user).