This is an archive of the discontinued LLVM Phabricator instance.

[libc++] [C++20] [P0415] Constexpr for std::complex.
ClosedPublic

Authored by philnik on May 7 2020, 12:17 AM.

Details

Summary

This patch adds constexpr to <complex> header: operators, member operators, and member functions (real, imag, norm, conj).

https://eel.is/c++draft/complex.numbers
https://wg21.link/p0415

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2020, 12:17 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Nice! Are you looking for a review now, or it's just a WIP patch? Ideally you'd fix the TODOs you already noted before we take a look, unless you have specific questions/concerns.

ldionne requested changes to this revision.May 7 2020, 10:28 AM

(Requesting changes so it shows up properly in my review queue)

This revision now requires changes to proceed.May 7 2020, 10:28 AM

@ldionne, I'd appreciate if you (or somebody else) could tell me what you think about the way I test the constexpr. I wanted to avoid duplication but on a second thought, I think that run-time tests can actually go through compile-time (constexpr) branch and so leave the non-constexpr branch untested.
Another thing in which I'd need guidance is how to test the primary implementation (not specializations for float, double, long double). Is there another floating-point type I can use in tests? Maybe some sort of short float?
Oh, and should integral types be tested with complex<T>?

@ldionne, I'd appreciate if you (or somebody else) could tell me what you think about the way I test the constexpr. I wanted to avoid duplication but on a second thought, I think that run-time tests can actually go through compile-time (constexpr) branch and so leave the non-constexpr branch untested.

I think it's fine -- avoiding duplication is pretty important IMO. I don't think the compiler will try to evaluate this using the constexpr evaluator if it's not marked as constexpr. If we wanted to be really really sure, we could also make sure the values we use in the test are thought to be potentially-runtime by calling a function defined in a different TU. That would be somewhat more complicated, but imagine the following setup:

// in support.cpp
template <typename T>
T identity(T t) { return t; }

template int identity(int);
template float identity(float);
// etc...

Then, let's say you have a test that is potentially constexpr like this:

template <class T>
TEST_CONSTEXPR_CXX20 bool test(T zero) {
    T lhs(1.5 + zero);
    std::complex<T> rhs(3.5 + zero, 4.5);
    std::complex<T>   x(5.0 + zero, 4.5);
    test(lhs, rhs, x);
}

You could now call it once at runtime and once at compile-time:

int main(int argc, char** argv) {
    test<float>(identity<float>(0)); // runtime test
    static_assert(test<float>(0)); // compile-time test
}

Since identity is defined in a different TU, the compiler would really have to generate code for the runtime tests. There's a few different ways to do this, another way I can think of is to piggy back on argc or just a (volatile?) global variable. But the idea is the same. If there's interest, we can look into setting this up more widely for amphibious (constexpr/not constexpr) tests.

Another thing in which I'd need guidance is how to test the primary implementation (not specializations for float, double, long double). Is there another floating-point type I can use in tests? Maybe some sort of short float?

I'm seeing this on cppreference:

The specializations std::complex<float>, std::complex<double>, and std::complex<long double> are LiteralTypes for representing and manipulating complex numbers. The effect of instantiating the template complex for any other type is unspecified.

I don't think we test other specializations in our other complex tests, do we? If not, I'd say it's OK to not test it... Maybe I'm not being imaginative enough?

Oh, and should integral types be tested with complex<T>?

Like I said above, I don't think so. Please LMK if you disagree.

What *does* need to be tested however is the interaction between std::complex<T> and U where T in {float, double, long double} and U is an integral type. But you seem to be testing this correct (e.g. in conj.pass.cpp).

libcxx/test/std/numerics/complex.number/complex.ops/scalar_plus_complex.pass.cpp
34

Nit: this spacing seems to have changed?

curdeius planned changes to this revision.May 7 2020, 3:49 PM
curdeius updated this revision to Diff 303663.Nov 7 2020, 12:53 PM

Fix edge cases. Fix operator* and operator/.
Use "shortcuts" to avoid NaN in constexpr evaluation (disallowed).

One remaining problem.
2 tests:

  • complex_divides_complex.pass.cpp
  • complex_times_complex.pass.cpp

do not pass on clang unless additional parameter --param=compile_flags='-fconstexpr-steps=4000000' is given.
That's because they test all the edge cases and each pair thereof and clang has too low a limit for the maximum number of constexpr execution steps.
I thought about splitting these tests somehow but given their (combinatoric) nature, it's pretty awkward.

I don't know how I should pass this compile flag only for these tests and only for clang (I know of ADDITIONAL_COMPILE_FLAGS but I cannot exclude gcc with that).

TODO:

  • Test (at least locally) a different floating-point type (e.g. float128) to catch missing constexpr markers in the default implementation (non-specialization).
curdeius updated this revision to Diff 303664.Nov 7 2020, 12:55 PM

Fix diff.

curdeius updated this revision to Diff 303665.Nov 7 2020, 12:59 PM
  • Revert unrelated changes in cases.h.
curdeius updated this revision to Diff 303669.Nov 7 2020, 1:29 PM
  • Add missing constexpr on generic implementation.
  • Add missing static asserts in tests.
curdeius updated this revision to Diff 303670.Nov 7 2020, 1:31 PM
  • Bump version in status.
curdeius updated this revision to Diff 303673.Nov 7 2020, 1:53 PM
  • Fix pre-C++20 modes. Ugly duplication :/.
curdeius updated this revision to Diff 303674.Nov 7 2020, 1:57 PM
  • Clean up.

I've tested the non-specialization with __float128 and fixed missing constexpr there. The tests mainly pass apart from those testing operators * and / that use copysign which is ambiguous (already in non-constexpr context so I didn't pursue it any further). It's implementation-specific anyway.

wmaxey added a subscriber: wmaxey.Nov 12 2020, 8:33 PM
ldionne requested changes to this revision.Nov 17 2020, 2:26 PM

Can you please ping once you think this is ready to review? For now, I'll "request changes" so it shows up correctly in the review queue.

This revision now requires changes to proceed.Nov 17 2020, 2:26 PM
curdeius retitled this revision from [libc++] [P0415] [C++20] - Constexpr for std::complex. to [libc++] [P0415] [C++20] Constexpr for std::complex..Nov 18 2020, 1:06 AM
curdeius edited the summary of this revision. (Show Details)
curdeius planned changes to this revision.Nov 18 2020, 1:21 AM
curdeius updated this revision to Diff 307159.Nov 23 2020, 12:08 PM
  • Hack to add a compile flag only to clang.
  • Rebase
curdeius updated this revision to Diff 307291.Nov 24 2020, 2:21 AM
  • Fix apple-clang.
  • Add header guards.

@ldionne, it's ready for review (at least the first glance).

ldionne requested changes to this revision.Feb 3 2021, 2:18 PM

Can you rebase onto main so that CI runs again? You'll also need to fix a few details like the cxx2a_status page which has changed.

libcxx/include/complex
860

I think we really need to unfork these implementations and use the same one in all standard modes. It looks like the only difference is pretty much that you're calling the _constexpr variations when is_constant_evaluated() in the C++20 version. Would it be possible to always call e.g. __libcpp_scalbn, and then have that function check for is_constant_evaluated() and use a constexpr-friendly implementation in that case?

This revision now requires changes to proceed.Feb 3 2021, 2:18 PM
curdeius updated this revision to Diff 321700.Feb 5 2021, 3:57 AM
  • Use __libcpp_is_constant_evaluated in *_constexpr helper functions.
  • Update status page and feature test macros.
  • Rebase.
curdeius updated this revision to Diff 321710.Feb 5 2021, 4:35 AM
  • Fix pre-C++14 builds.
curdeius planned changes to this revision.Feb 5 2021, 4:54 AM
curdeius marked an inline comment as done.
curdeius added inline comments.
libcxx/include/complex
586–615

I guess I need to do the same #if dance as below, so as not to change the current runtime behaviour.

601

Oops. Should be __builtin_copysign (without l).

760

Oops. Will remove in next update.

860

Done. The solution is IMO still a bit ugly (with those #ifdefs), but it's much better than before.
Thanks for the suggestion.

curdeius updated this revision to Diff 321722.Feb 5 2021, 5:35 AM
curdeius marked an inline comment as done.
  • Small fixes.
curdeius updated this revision to Diff 321733.Feb 5 2021, 6:10 AM
  • Revert whitespace changes.
curdeius marked an inline comment as done.Feb 5 2021, 6:14 AM
curdeius edited the summary of this revision. (Show Details)Feb 9 2021, 12:31 AM
curdeius retitled this revision from [libc++] [P0415] [C++20] Constexpr for std::complex. to [libc++] [C++20] [P0415] Constexpr for std::complex..Feb 24 2021, 1:49 AM
curdeius updated this revision to Diff 333519.Mar 26 2021, 3:05 AM

Ping.

  • Rebase.
  • Revert whitespace changes.
Quuxplusone added inline comments.
libcxx/include/complex
237–238

I think numeric_limits gives you the best chance of making things work generically with non-base-2 types (not that such things exist).
I've been starting to comment #include lines with the stuff we need them for; although I admit in this case it's pretty obvious what <limits> is always needed for ;)

601

The l is still there.

612

Here _LIBCPP_CONSTEXPR_AFTER_CXX17 can be just constexpr, because this code is enabled only when _LIBCPP_STD_VER > 17.

724

IIUC these two operator* implementations are identical except that the C++20 one has an extra block under __libcpp_is_constant_evaluated, and then also replaces copysign with __copysign_constexpr even outside that block.
Would it be possible to combine the operator* implementations through judicious use of #if (for the first diff) and #define _LIBCPP_COMPLEX_COPYSIGN __copysign_constexpr (for the second diff)?

(Also, my ADL senses are on high alert through this entire function, but I guess we expect _Tp always to be a primitive type, so ADL doesn't matter here.)

801

Above you used _Tp(0) for zero. I think _Tp(0) is clearer than _Tp{}, and you should use _Tp(0) down here too.

860

Whatever you did to "unfork" the implementations seems maybe to have been lost between February and late-March. I still see two largely identical implementations of operator/ here (like the two operator*s above).

libcxx/test/std/numerics/complex.number/cmplx.over/conj.pass.cpp
30

These three helper functions can stay returning void; only the test on line 55 needs to return bool.
(I do wish the helpers weren't named the same name as the top-level test.)
This comment applies to many of these test files.

libcxx/test/std/numerics/complex.number/complex.ops/complex_divide_complex.clang.pass.cpp
11 ↗(On Diff #333519)

The existing tests that use ADDITIONAL_COMPILE_FLAGS do it like this:

// REQUIRES: -faligned-allocation
// ADDITIONAL_COMPILE_FLAGS: -faligned-allocation

Could we do something similar here?

I also question the "hpp"-ness of complex_divide_complex.pass.hpp. It certainly shouldn't have a header guard against multiple inclusion; but I don't think its filename should involve either "hpp" (it's not a header) nor ".pass." (the filename isn't intended for parsing by the test runner).
Of course those naming issues all disappear if you are able to do REQUIRES: -fconstexpr-steps=4500000 here.

And of course that issue will disappear if you can figure out how to squeeze some extra "cycles" out of the constexpr algorithm and/or test case. See below.

libcxx/test/std/numerics/complex.number/complex.ops/complex_divide_complex.pass.hpp
35–36 ↗(On Diff #333519)

Incidentally, this is a very silly series of (pre-existing) helper functions. I think you could take this suggested edit and then delete lines 24-30.

This comment applies to many of these tests.

49 ↗(On Diff #333519)

I wonder whether the repeated calculation of classify is driving up your constexpr "cycle count"; maybe it could make sense to put at the top of this function

int classification[N];
for (unsigned i=0; i < N; ++i)
  classification[i] = classify(testcases[i]);

You're still doing O(N^2) divisions, though, which is probably the more salient problem, and harder to work around. :P

libcxx/test/std/numerics/complex.number/complex.ops/unary_plus.pass.cpp
28

This entire test could be

assert(+std::complex<T>(1.5, -2.5) == std::complex<T>(1.5, -2.5));
assert(+std::complex<T>(-1.5, 2.5) == std::complex<T>(-1.5, 2.5));
return true;

and ta-da, we've more than doubled our test coverage. :P

curdeius planned changes to this revision.Mar 26 2021, 10:21 AM

There's something fishy with my rebase.

curdeius updated this revision to Diff 333622.Mar 26 2021, 1:24 PM
curdeius marked 5 inline comments as done.

Fix rebase. Apply some review comments.

  • Revert whitespace changes.
  • Use numeric_limits.
  • Use _Tp(0).
curdeius updated this revision to Diff 333637.Mar 26 2021, 2:46 PM
curdeius marked 4 inline comments as done.
  • Simplify tests (mainly helper functions).
libcxx/test/std/numerics/complex.number/complex.ops/complex_divide_complex.clang.pass.cpp
11 ↗(On Diff #333519)

Using REQUIRES as you suggest would disable the tests for gcc... And I'm not sure we want this.

libcxx/test/std/numerics/complex.number/complex.ops/complex_divide_complex.pass.hpp
49 ↗(On Diff #333519)

Yeah, computing the result of classify once does drive the count down... it's almost ok for the multiplication (1.7M and the default clang's limit is 1M).
But it's still at 3.5M for the division.

I'm all ears if anyone has an idea how to remove this ugly .hpp hack (which only purpose is to pass -fconstexpr-steps=N to clang, and to clang only).
What I'd like is to do something like ADDITIONAL_COMPILE_FLAGS: (apple-clang || clang) -fconstexpr-steps=N or whatever else that does this.
I've tried different lit magic, but to no avail.

Looks like your new rebase succeeded in preserving the "unforked" implementations of operator* and operator/. They LGTM code-wise now.
No comment on the math part. It seems like complex_times_complex.pass.cpp does a pretty thorough job of testing its behavior, though, so I trust it.

libcxx/include/complex
792

My impression is that you don't need these dangling elses (lines 739, 750, 777) for any C++ reason (like, the else on line 739 is not saving you from having to resolve and instantiate fmax on line 741). Do they improve codegen? If not, I say eliminate them — splitting unbraced control-flow structures across #if/#endif in this way is not-good. (Consider what happens if you replace line 741 with two lines of code, for example.)

curdeius updated this revision to Diff 333776.Mar 28 2021, 11:55 PM
curdeius marked an inline comment as done.
  • Get rid of dangling "else"s.
libcxx/include/complex
792

Good catch. Eliminated.

curdeius added inline comments.Mar 28 2021, 11:58 PM
libcxx/include/complex
724

Concerning ADL, it's unspecified by the standard what happens if _Tp is other than float, double, long double. Even less if it's a non-primitive type :).

Friendly ping.

ldionne requested changes to this revision.Jun 24 2021, 9:13 AM

Sorry for sitting on this for so long. Let's push this through now.

libcxx/include/complex
586

We already jump through some hoops in <math.h> to provide copysign. Would it make sense to try and unify those in some way? Maybe create a __copysign function that is constexpr whenever it can be, and then call that from copysign. Here, we'd call __copysign directly, which has the right constexpr-ness. WDYT? I'm trying to avoid proliferating helper functions that might be useful elsewhere in just this header, since it means we'll reinvent the wheel in the future eventually.

Also, I don't understand why we're guarding the use of the builtin on _LIBCPP_STD_VER and not __has_builtin.

Same comment applies to other general functions below like fabs & friends. I think it may be a good idea to tackle those as a separate patch, then this patch would be a lot simpler.

642

I think we can remove the #if _LIBCPP_STD_VER > 17 check here. In C++ <= 17, __libcpp_is_constant_evaluated() will just return false, so this if will never be entered (but it still needs to parse).

Also, I believe a comment saying something like "catch all corner cases that would otherwise cause invalid operations if evaluated in a constant expression" wouldn't hurt. This applies here and everywhere else where the same pattern appears.

libcxx/test/std/numerics/complex.number/cases.h
182

We can't use builtins like that in the tests unless we know all supported compilers do support that builtin. Is it the case?

libcxx/test/std/numerics/complex.number/complex.ops/complex_divide_complex.clang.pass.cpp
11 ↗(On Diff #333776)

I'm quite worried by the need to add this. It means that if we somehow went crazy in the implementation and used a huge number of steps to implement the functionality, we wouldn't catch it with the tests. I'd like to suggest something like this to remove the need to pass -fconstexpr-steps:

// Helper to avoid blowing the number of steps in constexpr
template <typename F>
TEST_CONSTEXPR void chunk(std::size_t N, F f) {
  for (std::size_t i = (N/5) * 0; i != (N/5) * 1; ++i) f(i);
  for (std::size_t i = (N/5) * 1; i != (N/5) * 2; ++i) f(i);
  for (std::size_t i = (N/5) * 2; i != (N/5) * 3; ++i) f(i);
  for (std::size_t i = (N/5) * 3; i != (N/5) * 4; ++i) f(i);
  for (std::size_t i = (N/5) * 4; i != N;         ++i) f(i);
}

This is just an idea, and it's untested code, but basically I think we could add a generic helper to help us chunk large ranges into several for loops instead of one giant loop. Then we'd use it something like

TEST_CONSTEXPR_CXX20
bool test_edges() {
  const unsigned N = sizeof(testcases) / sizeof(testcases[0]);
  int classification[N];
  for (unsigned i=0; i < N; ++i)
    classification[i] = classify(testcases[i]);

  chunk(N, [](int i) {
    for (int j = 0; j != N; ++j) {
      std::complex<double> r = testcases[i] / testcases[j];
      switch (classification[i]) {
        ...
      };
    }
  });
}
This revision now requires changes to proceed.Jun 24 2021, 9:13 AM
curdeius added inline comments.Jul 21 2021, 7:50 AM
libcxx/test/std/numerics/complex.number/complex.ops/complex_divide_complex.clang.pass.cpp
11 ↗(On Diff #333776)

I'm not sure I understand what you propose.
The fact that we go over the max. constexpr steps, is because of static_assert(test_edges());.
Which is a single constexpr expression to compute.
And so, even splitting up testcases to 20 pieces still hits the limit on clang (obviously with the default constexpr-steps limit).
It has as well a disadvantage of reducing the test coverage.

On the other hand, I agree that all this file splitting is a burden.
Would it be doable to have sth like // ADDITIONAL_COMPILE_FLAGS_IF_SUPPORTED: -fconstexpr-steps=3500000?

ldionne added inline comments.Jul 28 2021, 8:37 AM
libcxx/docs/Cxx2aStatusPaperStatus.csv
8 ↗(On Diff #333776)
libcxx/test/std/numerics/complex.number/complex.ops/complex_divide_complex.clang.pass.cpp
11 ↗(On Diff #333776)

Hmmmmm, yeah, I guess my suggestion was quite silly in retrospect. I didn't realize the constexpr step limit was on the overall constexpr evaluation, but that's the only thing that makes sense now that I think of it. Let's take this discussion to D106798.

philnik commandeered this revision.Dec 15 2022, 7:57 AM
philnik added a reviewer: curdeius.
philnik added a subscriber: philnik.

Commandeering to finish this, since it's been laying around for over a year and it seems like it's almost finished.

libcxx/include/complex
586

We already jump through some hoops in <math.h> to provide copysign. Would it make sense to try and unify those in some way? Maybe create a __copysign function that is constexpr whenever it can be, and then call that from copysign. Here, we'd call __copysign directly, which has the right constexpr-ness. WDYT? I'm trying to avoid proliferating helper functions that might be useful elsewhere in just this header, since it means we'll reinvent the wheel in the future eventually.

Also, I don't understand why we're guarding the use of the builtin on _LIBCPP_STD_VER and not __has_builtin.

Same comment applies to other general functions below like fabs & friends. I think it may be a good idea to tackle those as a separate patch, then this patch would be a lot simpler.

Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2022, 7:57 AM
philnik updated this revision to Diff 483195.Dec 15 2022, 8:03 AM
philnik marked 7 inline comments as done.
  • Rebase
  • Address comments
libcxx/include/complex
586

I think with the refactoring of math.h this comment doesn't apply anymore.

libcxx/test/std/numerics/complex.number/complex.ops/complex_divide_complex.clang.pass.cpp
11 ↗(On Diff #333776)

Doesn't apply anymore, since we have mechanisms for increasing constexpr execution limits now.

philnik updated this revision to Diff 483196.Dec 15 2022, 8:05 AM

Remove old status paper

ldionne accepted this revision.Dec 20 2022, 4:50 PM

LGTM w/ green CI and assuming the answer to my question is "yes" (i.e. you didn't slip a change to the non-constexpr code path).

libcxx/include/complex
883–936

Can you confirm that this is the old implementation, we only changed calls to foo to __constexpr_foo?

This revision is now accepted and ready to land.Dec 20 2022, 4:50 PM
philnik updated this revision to Diff 485228.Dec 25 2022, 7:28 AM
philnik marked an inline comment as done.

Try to fix CI

libcxx/include/complex
883–936

Yes.

philnik updated this revision to Diff 485336.Dec 26 2022, 7:32 PM

Try to fix CI

philnik updated this revision to Diff 485369.Dec 27 2022, 4:28 AM

Try to work around windows bugs

philnik updated this revision to Diff 487158.Jan 8 2023, 3:59 AM

Replace INFINITY with numeric_limits<_Tp>::infinity()

philnik updated this revision to Diff 487161.Jan 8 2023, 4:39 AM

Replace INFINITY and NAN in more places

This revision was automatically updated to reflect the committed changes.
jdoerfert reopened this revision.Jan 10 2023, 12:06 AM
jdoerfert added subscribers: tra, jdoerfert.

FWIW, this broke the CUDA buildbots: https://lab.llvm.org/buildbot/#/builders/1/builds/41521

Tag: @tra

This revision is now accepted and ready to land.Jan 10 2023, 12:06 AM

FWIW, this broke the CUDA buildbots: https://lab.llvm.org/buildbot/#/builders/1/builds/41521

Tag: @tra

That looks to me like a problem with the cuda wrappers, given that we don't support cuda and we don't have any __host__ or __device__ tags.

Thanks for the heads up @jdoerfert.

It looks like Clang-cuda is re-defining <complex> in clang/lib/Headers/cuda_wrappers/complex. I'm not sure how that works but that is likely the issue. I'd wait to hear back from the Clang-cuda folks what they think the exact issue is before considering a revert, since it really seems to me like libc++ isn't doing anything wrong.

tra added a comment.Jan 11 2023, 3:30 PM

Thanks for the heads up @jdoerfert.

It looks like Clang-cuda is re-defining <complex> in clang/lib/Headers/cuda_wrappers/complex. I'm not sure how that works but that is likely the issue. I'd wait to hear back from the Clang-cuda folks what they think the exact issue is before considering a revert, since it really seems to me like libc++ isn't doing anything wrong.

I'm aware of the broken bots, just didn't get a chance to deal with them yet.

It's not a libc++ problem, so I don't think we need to revert.

CUDA headers need to grow additional magic to make new libc++ code work on the GPU side. It's unfortunate that we need to play catch up here, but short of introducing CUDA support to libc++ itself, there's not much else we can do.

I'm a bit puzzled why things didn't work, though, as normally CUDA compilation should've treated constexpr functions as __host__ __device__, and should've allowed __constexpr_fmax() etc... It's possible that we may have triggered yet another new corner case.

tra added a comment.Jan 11 2023, 5:24 PM

Something like https://reviews.llvm.org/D141555 should unbreak CUDA. It can probably get reduced a bit, as we may not have sufficiently functional builtins on the GPU, so it's still a WIP.

I can't say that I like copy/pasting libc++ code into clang headers. I think it would be great if we could directly add necessary attributes to specific functions when libc++ headers are compiled as CUDA.

libcxx/include/cmath
740 ↗(On Diff #487181)

Nit: this should be // !__has_constexpr_builtin(__builtin_logb)

781 ↗(On Diff #487181)

Nit: // !has_constexpr_builtin(builtin_scalbln)

philnik closed this revision.Jan 11 2023, 5:49 PM
philnik marked 2 inline comments as done.
In D79555#4045429, @tra wrote:

CUDA headers need to grow additional magic to make new libc++ code work on the GPU side. It's unfortunate that we need to play catch up here, but short of introducing CUDA support to libc++ itself, there's not much else we can do.

If you're interested in doing that you can post an RFC and tell us what would be required for that.

Closing this, since it doesn't seem to be a libc++ problem.

libcxx/include/cmath
781 ↗(On Diff #487181)