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

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius added inline comments.


================
Comment at: libcxx/test/std/numerics/complex.number/complex.ops/complex_divide_complex.clang.pass.cpp:11
+
+// ADDITIONAL_COMPILE_FLAGS: -fconstexpr-steps=4500000
+
----------------
Quuxplusone wrote:
> 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.
Using `REQUIRES` as you suggest would disable the tests for gcc... And I'm not sure we want this.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79555