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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/complex:585
 
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17 float
+__copysign_constexpr(float __lcpp_x, float __lcpp_y) {
----------------
philnik wrote:
> ldionne wrote:
> > 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.
> > 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.
> 
> 
I think with the refactoring of `math.h` this comment doesn't apply anymore.


================
Comment at: libcxx/test/std/numerics/complex.number/complex.ops/complex_divide_complex.clang.pass.cpp:11
+
+// ADDITIONAL_COMPILE_FLAGS: -fconstexpr-steps=3500000
+
----------------
ldionne wrote:
> curdeius wrote:
> > ldionne wrote:
> > > 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]) {
> > >         ...
> > >       };
> > >     }
> > >   });
> > > }
> > > ```
> > 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`?
> 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.
Doesn't apply anymore, since we have mechanisms for increasing `constexpr` execution limits now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79555