Page MenuHomePhabricator

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

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

Details

Reviewers
ldionne
EricWF
mclow.lists
Group Reviewers
Restricted Project
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
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
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?

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
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 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.Fri, Mar 26, 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 ;)

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 planned changes to this revision.Fri, Mar 26, 10:21 AM

There's something fishy with my rebase.

curdeius updated this revision to Diff 333622.Fri, Mar 26, 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.Fri, Mar 26, 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
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.

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 updated this revision to Diff 333776.Sun, Mar 28, 11:55 PM
curdeius marked an inline comment as done.
  • Get rid of dangling "else"s.
libcxx/include/complex
739

Good catch. Eliminated.

curdeius added inline comments.Sun, Mar 28, 11:58 PM
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 :).

Friendly ping.