[libcxx-commits] [PATCH] D79555: [libc++] [C++20] [P0415] Constexpr for std::complex.

Fri Mar 26 09:19:21 PDT 2021

Quuxplusone added inline comments.

================
Comment at: libcxx/include/complex:237-238
 #include <stdexcept>
+// TODO: Should we use FLT_RADIX from <cfloat> or __FLT_RADIX__ from <limits> or numeric_limits<T>::radix from <limits>?
+#include <limits>
 #include <cmath>
----------------
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 ;)

================
Comment at: libcxx/include/complex:612
+template <class _Tp>
+_LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17 _Tp
+__abs_constexpr(_Tp __lcpp_x) _NOEXCEPT {
----------------
Here `_LIBCPP_CONSTEXPR_AFTER_CXX17` can be just `constexpr`, because this code is enabled only when `_LIBCPP_STD_VER > 17`.

================
Comment at: libcxx/include/complex:724
+}
+#else
 template<class _Tp>
----------------
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.)

================
Comment at: libcxx/include/complex:825
+  if (__libcpp_is_constant_evaluated()) {
+    return (__lcpp_x < _Tp{}) ? -__lcpp_x : __lcpp_x;
+  } else {
----------------
Above you used `_Tp(0)` for zero. I think `_Tp(0)` is clearer than `_Tp{}`, and you should use `_Tp(0)` down here too.

================
Comment at: libcxx/include/complex:601
+{
+    return __builtin_copysignl(__lcpp_x, __lcpp_y);
+}
----------------
curdeius wrote:
> Oops. Should be `__builtin_copysign` (without `l`).
The `l` is still there.

================
Comment at: libcxx/include/complex:921
+complex<_Tp>
+operator/(const complex<_Tp>& __z, const complex<_Tp>& __w)
+{
----------------
curdeius wrote:
> ldionne wrote:
> > 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?
> Done. The solution is IMO still a bit ugly (with those #ifdefs), but it's much better than before.
> Thanks for the suggestion.
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).

================
Comment at: libcxx/test/std/numerics/complex.number/cmplx.over/conj.pass.cpp:30
+  assert(std::conj(x) == conj(std::complex<double>(x, 0)));
+  return true;
 }
----------------
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.

================
Comment at: libcxx/test/std/numerics/complex.number/complex.ops/complex_divide_complex.clang.pass.cpp:11
+
+// ADDITIONAL_COMPILE_FLAGS: -fconstexpr-steps=4500000
+
----------------
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.

================
Comment at: libcxx/test/std/numerics/complex.number/complex.ops/complex_divide_complex.pass.hpp:35-36
+  std::complex<T> rhs(1.5, 2.5);
+  std::complex<T> x(1.5, 2.5);
+  test(lhs, rhs, x);
+  return true;
----------------
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.

================
Comment at: libcxx/test/std/numerics/complex.number/complex.ops/complex_divide_complex.pass.hpp:49
+        case zero:
+          assert(classify(r) == NaN);
+          break;
----------------
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

================
Comment at: libcxx/test/std/numerics/complex.number/complex.ops/unary_plus.pass.cpp:28
+  assert(c.imag() == T(2.5));
+  return true;
 }
----------------
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

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79555/new/

https://reviews.llvm.org/D79555