This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement C++20's P0476r2: std::bit_cast
ClosedPublic

Authored by ldionne on Mar 10 2020, 1:43 PM.

Diff Detail

Event Timeline

ldionne created this revision.Mar 10 2020, 1:43 PM
Herald added a project: Restricted Project. · View Herald Transcript
EricWF requested changes to this revision.Mar 11 2020, 8:12 PM

LGTM other than the lack of tests.

We should also figure out what GCC's plan is here, so we can implement a GCC version in future.

libcxx/include/bit
366

We should probably mark this nodiscard.

371

Gosh. __builtin_bit_cast is a weird builtin -- taking a type as the first parameter.

373

#endif // !defined(_LIBCPP_HAS_NO_BUILTIN_BIT_CAST)

libcxx/test/std/numerics/bit/bit.cast/bit_cast.fail.cpp
58

Are these errors emitted inside <bit>? If so, we should specify that here.

libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
24

What's the fun stuff?

This revision now requires changes to proceed.Mar 11 2020, 8:12 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 11 2020, 8:12 PM
ldionne updated this revision to Diff 250243.Mar 13 2020, 9:54 AM
ldionne marked 5 inline comments as done.

Add tests

ldionne updated this revision to Diff 250245.Mar 13 2020, 9:58 AM
ldionne marked 3 inline comments as done and an inline comment as not done.

Specify that the errors from __builtin_bit_cast come from <bit>

ldionne added inline comments.
libcxx/include/bit
366

Marked it with _LIBCPP_NODISCARD_EXT, which we use for nodiscard-as-an-extension.

368

Question: the spec has these as Constraints: -- I think it means these should be used to SFINAE out, not hard-error -- right?

libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
24

Writing the tests.

154

@erik.pilkington Any idea why those don't work inside constexpr?

158

@jfb Can you think of other interesting values to test?

197

@erik.pilkington All the tests for long double are failing (at runtime) -- any idea why? Did you implement support for long double in __builtin_bit_cast?

ldionne edited the summary of this revision. (Show Details)Mar 13 2020, 9:59 AM
libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
154

Its hitting the isInt() check here: https://github.com/llvm/llvm-project/blob/master/clang/lib/AST/ExprConstant.cpp#L11102. Seems like constexpr __builtin_memcmp was only implemented for integers.

197

Eek, it looks like this is asserting at compile time too. Let me take a closer look.

jfb added inline comments.Mar 13 2020, 10:52 AM
libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
165

Signaling NaN, and NaNs with payloads.
Infinities.
Denormals.

Check this out: https://babbage.cs.qc.cuny.edu/IEEE-754/index.xhtml

195

Some of these won't work on all platforms. You'll need to selectively disable some for platforms that don't have IEEE-754 with the expected sizes.

libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
197

I put a patch up to fix this here: https://reviews.llvm.org/D76323

tambre added a subscriber: tambre.Jun 20 2020, 4:19 AM
tambre added inline comments.Jun 20 2020, 4:29 AM
libcxx/include/bit
368

Correct.

From [structure.specifications] 3.1:

Constraints: the conditions for the function’s participation in overload resolution.

chfast added a subscriber: chfast.Dec 8 2020, 1:57 PM

This approach may be frowned upon because std::bit_cast is implemented with __builtin_bit_cast but, I think you could actually use __builtin_bit_cast to write the comparison function you want for the tests:

template<class T> struct Buff { char buff[sizeof(T)]; };

template<class T>
constexpr bool cmp(const T &a, const T &b) {
    auto c1 = __builtin_bit_cast(Buff<T>, a);
    auto c2 = __builtin_bit_cast(Buff<T>, b);
    for (unsigned i = 0; i < sizeof(T); ++i) {
        if (c1.buff[i] != c2.buff[i])
            return false;
    }
    return true;
}
libcxx/test/std/numerics/bit/bit.cast/bit_cast.fail.cpp
32

Maybe a deleted copy constructor?

libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
44

So, in non-constexpr, the only thing you're testing here is that you can cast a pointer to its current type?

ldionne updated this revision to Diff 316245.Jan 12 2021, 2:39 PM
ldionne marked 8 inline comments as done.

Rebase onto main and update tests + implementation.

The tests are still not passing, and I have to verify whether it's a Clang
bug or whether the tests are wrong.

libcxx/test/std/numerics/bit/bit.cast/bit_cast.fail.cpp
32

I think that's covered by the test for a type that isn't trivially copyable. If a type is not copyable, then it is not trivially copyable.

libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
44

I don't understand. In constexpr mode, the only thing being tested is that the roundtrip from T back to T (through std::bit_cast) works as expected. I'm only unable to test the intermediate results (the char arrays).

However, as of rebasing this patch, it appears that none of it works anymore. So I don't have a way to test bit_cast inside constexpr.

195

I'll see what CI complains about!

zoecarver added inline comments.Jan 12 2021, 3:19 PM
libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
44

This comment was referring to the old patch where there was an is_constant_evaluated condition in the test_buffer_roundtrip function.

209

I assume this means you're against using __builtin_bit_cast to test std::bit_cast? Fair enough.

ldionne marked an inline comment as done.Jan 13 2021, 7:59 AM
ldionne added inline comments.
libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
209

Yes, because std::bit_cast is implemented as a single call to __builtin_bit_cast. I feel like what we're actually testing here is in fact __builtin_bit_cast directly, not just std::bit_cast.

curdeius added inline comments.
libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
154

Nit: you should probably use long long instead of long so that this test doesn't break on LLP64 data model (Windows). You might use uint64_t as well.

curdeius added inline comments.Jan 13 2021, 2:54 PM
libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
50

Not sure if this adds much value, but can't you additionally test assert(from == middle.x);?
Similarly for middle2.

zoecarver added inline comments.Jan 13 2021, 9:37 PM
libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
209

Fair enough. Ideally, Clang's tests would be sufficient, but I see where you're coming from. I think that makes sense.

Would it make it better if the call to __builtin_bit_cast always casted it to a char array and compared the bytes? That would basically be re-implementing memcmp. If we can verify that the cast to char array always works, then we can use that to test other things, maybe.

zoecarver added inline comments.Jan 13 2021, 11:12 PM
libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
209

Here's a Godbolt. I think the issue is just when you try to compare different pointer types with __builtin_memcmp. I guess the earlier approach where you turned off certain asserts in constexpr mode actually makes sense.

https://godbolt.org/z/ozKn8W

Re [[nodiscard]]: hooray!
D99895 provides a Test Ready to Eat for the nodiscardness of bit_cast; once it's landed, you should be able to just rebase, uncomment that test, and add bit_cast to the appropriate list in UsingLibcxx.rst.

ldionne marked 4 inline comments as done.Jul 23 2021, 12:10 PM
ldionne added inline comments.
libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
50

Since I'm already testing memcmp, I feel like adding an equality test is not super useful. If the bytes are the same, they have to compare equal.

ldionne updated this revision to Diff 361296.Jul 23 2021, 12:11 PM
ldionne marked an inline comment as done.

Rebase onto main. Tests are still going to fail because Clang's __builtin_bit_cast still seems to have issues (I'll try to investigate those in the next week).

Quuxplusone requested changes to this revision.Jul 23 2021, 1:01 PM

LGTM % comments, FWIW.

libcxx/include/CMakeLists.txt
96–98 ↗(On Diff #361296)

Nit: / < _ < a according to ASCII.
But note that we get this wrong for __functional_base vs. __functional/... below. So if you take this nit, maybe fix both places; and/or maybe wonder are we historically sorting by something other than ASCIIbetically?

libcxx/include/__bit/bit_cast.h
29 ↗(On Diff #361296)

Please _LIBCPP_NODISCARD_EXT, and uncomment the existing test in libcxx/test/libcxx/diagnostics/nodiscard_extensions.pass.cpp which was added by D99895.

Please also add bit_cast to the list in libcxx/docs/UsingLibcxx.rst.

Please also git grep bit_cast libcxx/ and see if there are any more things waiting to be uncommented by this patch.

31 ↗(On Diff #361296)

Surely this should be guarded by __has_builtin(__builtin_bit_cast). Or is the rationale that we don't need to guard it, because we have no fallback implementation, and so "you get a weird compiler error" is already the best-case outcome on compilers that don't support it? Notice that because _ToType is a type, not a value, you'll get a parse error very eagerly as soon as <bit> is included; it's not merely that you'll get an error when you try to call bit_cast.

It looks like the builtin is supported on Clang 9+ and GCC 11+; it's unsupported on Clang 8- and GCC 10-.

libcxx/test/std/numerics/bit/bit.cast/bit_cast.compile.pass.cpp
36 ↗(On Diff #361296)

Consider adding things like

static_assert(!bit_cast_is_valid<char[2], From>);  // makes bit_cast's return type invalid
static_assert(!bit_cast_is_valid<int(), From>);  // duh, but might as well check
static_assert(bit_cast_is_valid<From, From>);  // OK, right?
static_assert(!bit_cast_is_valid<From&, From>);  // Not OK???

I'm not sure about the last one. Obviously real production code should use reinterpret_cast, not bit_cast, for such an operation; but I don't know if bit_cast is supposed to accept it or not.

49 ↗(On Diff #361296)

From() { } is superfluous, right? Remove.
Consider removing the function bodies, too.

libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
95

Here you're missing signed char, and below you're missing unsigned char. Also all the charN_t types, not that I'm asking for those.

110

I'm sure there must be supported platforms where long and double are different sizes. (If Linux isn't one, then Windows is probably one; and vice versa.)
...ah, I see, this line simply belongs on line 114 (under long long) instead. Ditto line 132 should be moved to 137.

This revision now requires changes to proceed.Jul 23 2021, 1:01 PM
Mordante added inline comments.
libcxx/docs/Status/Cxx20Papers.csv
32 ↗(On Diff #361296)

This should now become 14.

libcxx/include/__bit/bit_cast.h
28 ↗(On Diff #361296)

Would it make sense to use concepts instead of _EnableIf?

31 ↗(On Diff #361296)

AFAIK all supported platforms, including MSVC now support __builtin_bit_cast.

ldionne updated this revision to Diff 371445.Sep 8 2021, 2:24 PM
ldionne marked 12 inline comments as done.

Fix the tests (thanks Arthur)!

libcxx/include/CMakeLists.txt
96–98 ↗(On Diff #361296)

This is weird indeed, but I'm using VS Code's line sorting functionality. Will leave as-is since otherwise we'll just re-order stuff all the time because we use different sorts.

libcxx/include/__bit/bit_cast.h
28 ↗(On Diff #361296)

We generally use enable_if for Constraints: in the Standard, so I think I'd stick with that unless there is a benefit in using concepts here.

LGTM, just some nitpicky irrelevancies. :) If you're happy with the (lack of) constexpr tests, I say ship it already.

libcxx/docs/UsingLibcxx.rst
305 ↗(On Diff #371445)

I put this down between as_const and forward, which is a-z among the whatever-paper-that-was casting operators, instead of up here with the STL algorithms. bit_cast is not an STL algorithm.

libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
39

I'd checked from,middle and middle,to, in which case transitivity gets us from,to.
(This is just straight comparison of arrays of bytes, so no evil type can trick us here; transitivity definitely applies.)

40

By definition, sizeof(Buffer) equals sizeof(T), or else bit_cast would not compile... right?
I preferred to consistently use T throughout (and below instead of Nested), but I guess it doesn't matter.

254

I guess I meant to try to constexpr-ify tests() but never succeeded. The problems were (I think) with std::nanl and std::memcmp. I think it would be fine to ship this with only the basic_constexpr_test; although you might also try putting the memcmps under if (!std::is_constant_evaluated()) and splitting out the floating-point nan tests into a non-constexpr special test.

jfb added inline comments.Sep 8 2021, 4:54 PM
libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
254

nan will be constexpr if you do bool my_is_nan(double value) { return value != value; }.

ldionne updated this revision to Diff 371572.Sep 9 2021, 6:13 AM
ldionne marked 5 inline comments as done.

Address review comments and try fixing tests on ARM64

ldionne added inline comments.Sep 9 2021, 6:15 AM
libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
254

As Arthur says, the issue here is really to get a NaN value in constexpr context, which I don't know how to do. The other issue, which is more fundamental, is that we can't use std::memcmp in a constexpr context. And we can't use __builtin_memcmp, because although it *is* constexpr, it can't be used to compare different types, and we can't reinterpret_cast in a constexpr context.

ldionne accepted this revision.Sep 9 2021, 8:05 AM
This revision was not accepted when it landed; it landed in state Needs Review.Sep 9 2021, 8:06 AM
This revision was automatically updated to reflect the committed changes.
Quuxplusone added inline comments.Sep 9 2021, 8:11 AM
libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
254

Somewhat circular, but you can define a constexpr-evaluable memcmp in terms of bit_cast. :)
https://godbolt.org/z/49dhW4jzz
The only trick is that it can't take void*, void*, size_t; it must take T*, T*, because even bit_cast doesn't let you cast void* back to SomeOtherType* in a constexpr context.

From my perspective, the lack of memcmp isn't so fundamental, because we can just if out those tests when it's constexpr time. The more awkward programming problem is lines 220-225, where we need to generate an array with a couple different NaNs if it's non-constexpr, but somehow omit generating those array elements when it's constexpr time.
See my solution at D107036 (search for the phrase "specifically positive or negative NAN").