[libcxx-commits] [PATCH] D110738: [libc++] [P1614] Implement the first half of [cmp.alg]: strong_order, weak_order, partial_order.

[Kent Ross via Phabricator via libcxx-commits]

mumbleskates added inline comments.


================
Comment at: libcxx/include/__compare/strong_order.h:96
+                >;
+                static_assert(!std::is_same_v<_IntType, void>, "std::strong_order is unimplemented for this floating-point type");
+                if (__e_is_nan && __f_is_nan) {
----------------
Quuxplusone wrote:
> mumbleskates wrote:
> > Mordante wrote:
> > > Quuxplusone wrote:
> > > > Mordante wrote:
> > > > > I'm not familiar with this floating point code. But wouldn't this assert trigger on system where `sizeof(int) == 8 && sizeof(float) == 4`. Isn't it possible to test with `int32_t` and `int64_t`?
> > > > This assert //would// trigger on such systems, if any such systems exist.
> > > > On the one hand, I can't think of any reason for `int` to be 64-bit, unless `char` is also 64-bit, in which case `float` must also be at least 64-bit. So it makes more sense to wait and see if any such systems use libc++, and then if they do, they can submit a patch to use (i.e. `conditional_t` against) their particular type that works.
> > > > On the other hand, I actually //did// use `int32_t` and `int64_t` on lines 52–58, so why do I switch back to the built-in types here? I forget. It's particularly weird that I'm testing //three// types when in practice they'll only have //two// different sizes (32 and 64). So probably I should just change this to test only `int32_t` and `int64_t`.
> > > > I dunno; anyone else want to be the tiebreaker?
> > > I prefer the `int32_t` and `int64_t` especially since you used that earlier in the code.
> > Would it also be sensible for us to `static_assert` that `quiet_NaN()` and `signaling_NaN()` are ordered correctly by `strong_order`, or even perhaps SFINAE via a wrapper function that participates only when the behavior is as expected?
> > Would it also be sensible for us to `static_assert` that `quiet_NaN()` and `signaling_NaN()` are ordered correctly by `strong_order`
> 
> Isn't that covered in the tests below? (I'll comment on the exact line.)
I was thinking about in the library itself, such that the assertion runs at compile time for the user rather than only in our test suite's platform coverage. If that isn't part of our mandate of rigor and our CI tests are adequate for deploying the code everywhere then I suppose that would answer my question.


================
Comment at: libcxx/test/std/language.support/cmp/cmp.alg/strong_order.pass.cpp:348-351
+    // There's no way to produce a specifically positive or negative NAN
+    // at compile-time, so the NAN-related tests must be runtime-only.
+    // Also, x86-32 (x87 floating point) cannot handle signaling NANs;
+    // see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57484 for context.
----------------
Quuxplusone wrote:
> mumbleskates wrote:
> > Now that we have `bit_cast`, is it not possible for us to flip the sign by round tripping it it through an integer and xor'ing it with the bits for `-0.0` at compile time? (I am pretty sure that -0.0 has, in all iec559 encodings binary and decimal, only the sign bit set and all others zeroed out, which can also be checked via `has_single_bit`).
> It appears that we can! https://godbolt.org/z/z163Ges6G
> However, this requires a bit of metaprogramming to determine an integral type with the same width as `F`, and that's a bit more complicated than I really want to put into this test file. It would basically be repeating the `conditional_t` chain from lines 91–94 of `strong_order.h`.
> ```
> auto copysign = [](F f, int sign) {
>     using I = std::conditional_t<sizeof(F) == sizeof(int32_t), int32_t,
>         std::conditional_t<sizeof(F) == sizeof(int64_t), int64_t, void>>;
>     I mask = std::bit_cast<I>(F(-0.0));
>     if (sign < 0) {
>         return std::bit_cast<F>(std::bit_cast<I>(to) | mask);
>     } else {
>         return std::bit_cast<F>(std::bit_cast<I>(to) & ~mask);
>     }
> };
> ```
Okay, that's pretty cool! and could be helpful, depending on your thoughts on my other comments here.


================
Comment at: libcxx/test/std/language.support/cmp/cmp.alg/strong_order.pass.cpp:386
+#ifndef TEST_BUGGY_SIGNALING_NAN
+        assert(std::strong_order(ps, pq) == std::strong_ordering::less);
+#endif
----------------
Quuxplusone wrote:
> @mumbleskates: This is the exact line that compares `quiet_NaN()` against `signaling_NaN()` (and see line 395 too).
> Notice that we're testing "positive quiet NaN" (which might not be //literally// `quiet_NaN()`) against "positive signaling NaN" (which might not be //literally// `signaling_NaN()`), and lines 361 and 370 are testing "negative quiet" against "negative signaling"; we never literally test the `numeric_limits` values //directly//. The Standard doesn't mandate (AFAIK) whether `numeric_limits<F>::quiet_NaN()` should have a positive or negative sign bit.
Yeah, this all makes sense. I was mostly thinking in terms of getting this to also run statically, or check at our `strong_ordering` header's compile time whether these basic assumptions about the natural ordering of float payloads hold. Again not sure whether that's within our mandate, but recall that the iec559 standard //recommends//, but does not //require//, that the signaling bit be set to 1 for quiet and 0 for signaling; it //may// be reversed.

So yes, this also depends on how paranoid we are being. The only arch I have read about doing this for sure is PA-RISC, whose most recently released CPU came out in 2005, and any support for that platform is "out-of-tree" for LLVM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110738