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

[curdeius]

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

[curdeius]

• Revert unrelated changes in cases.h.

[curdeius]

• Add missing constexpr on generic implementation.
• Add missing static asserts in tests.

[curdeius]

• Bump version in status.

[curdeius]

• Fix pre-C++20 modes. Ugly duplication :/.

[curdeius]

• Clean up.

[curdeius]

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.

[ldionne]

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.

[curdeius]

• Hack to add a compile flag only to clang.
• Rebase

[curdeius]

• Fix apple-clang.
• Add header guards.

[curdeius]

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

[ldionne]

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

807

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?

[curdeius]

• Use __libcpp_is_constant_evaluated in *_constexpr helper functions.
• Update status page and feature test macros.
• Rebase.

[curdeius]

• Fix pre-C++14 builds.

[curdeius]

libcxx/include/complex

585–614

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

600

Oops. Should be __builtin_copysign (without l).

707

Oops. Will remove in next update.

807

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

[curdeius]

• Small fixes.

[curdeius]

• Revert whitespace changes.

[curdeius]

Ping.

• Rebase.
• Revert whitespace changes.

[Quuxplusone]

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

600

The l is still there.

611

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

723

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

748

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

807

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

35

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

12

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

36–37

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.

50

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

32

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]

There's something fishy with my rebase.

[curdeius]

Fix rebase. Apply some review comments.

• Revert whitespace changes.
• Use numeric_limits.
• Use _Tp(0).

[curdeius]

• Simplify tests (mainly helper functions).

libcxx/test/std/numerics/complex.number/complex.ops/complex_divide_complex.clang.pass.cpp

12

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

50

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.

[Quuxplusone]

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

739

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]

• Get rid of dangling "else"s.

libcxx/include/complex

739

Good catch. Eliminated.

[curdeius]

libcxx/include/complex

723

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

[curdeius]

Friendly ping.