This is an archive of the discontinued LLVM Phabricator instance.

[libc] Use __builtin_bit_cast only when src and dest types are trivially copyable
ClosedPublic

Authored by mikhail.ramalho on Apr 19 2023, 1:04 PM.

Details

Summary

This patch fixes the following compilation error:

/home/mgadelha/tools/llvm-project/libc/src/__support/CPP/bit.h:29:10: error: __builtin_bit_cast source type must be trivially copyable
  return __builtin_bit_cast(To, from);
         ^
/home/mgadelha/tools/llvm-project/libc/src/__support/FPUtil/FPBits.h:119:47: note: in instantiation of function template specialization '__llvm_libc::cpp::bit_cast<long double, __llvm_libc::cpp::UInt<128>>' requested here
  LIBC_INLINE T get_val() const { return cpp::bit_cast<T>(bits); }

This happen when building for a 32 bit target (in my case it was
riscv32) without int128 support, so we use UInt<128> instead, which
is not trivially copyable.

Diff Detail

Event Timeline

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 19 2023, 1:04 PM
mikhail.ramalho requested review of this revision.Apr 19 2023, 1:04 PM
mikhail.ramalho edited the summary of this revision. (Show Details)Apr 19 2023, 1:06 PM
sivachandra added inline comments.Apr 19 2023, 1:26 PM
libc/src/__support/CPP/bit.h
15

We cannot use the C++ standard library in the runtime components of the libc. Would marking the copy constructor with = default solve your problem: https://github.com/llvm/llvm-project/blob/main/libc/src/__support/UInt.h#L40

libc/src/__support/CPP/bit.h
15

Not really... We get the following error:

/home/root/llvm-project/libc/src/__support/CPP/bit.h:29:10: error: __builtin_bit_cast destination type must be trivially copyable
  return __builtin_bit_cast(To, from);
         ^
/home/root/llvm-project/libc/src/__support/FPUtil/FPBits.h:111:50: note: in instantiation of function template specialization '__llvm_libc::cpp::bit_cast<__llvm_libc::cpp::UInt<128>, long double>' requested here
  constexpr explicit FPBits(XType x) : bits(cpp::bit_cast<UIntType>(x)) {}
                                                 ^
/home/root/llvm-project/libc/src/__support/FPUtil/NearestIntegerOperations.h:56:13: note: in instantiation of function template specialization '__llvm_libc::fputil::FPBits<long double>::FPBits<long double, 0>' requested here
  FPBits<T> bits(x);
            ^
/home/root/llvm-project/libc/src/math/generic/ceill.cpp:16:18: note: in instantiation of function template specialization '__llvm_libc::fputil::ceil<long double, 0>' requested here
  return fputil::ceil(x);

Maybe can use the compiler builtin __is_trivially_copyable (like https://github.com/llvm/llvm-project/blob/main/libcxx/include/__type_traits/is_trivially_copyable.h) and add it to libc/src/__support/CPP/type_traits.h? WDYT?

Remove C++ standard library in the runtime components of the libc

asb added a comment.Apr 19 2023, 10:56 PM

In case it's helpful, I'll note you can force enable int128 support for rv32 clang with -fforce-enable-int128. This is a bit of a hacky solution to allow building compiler-rt for RV32 (which relies heavily in int128_t for its long double routines - I believe this wasn't an issue for other 32-bit architectures as RV32 is fairly unique in having sizeof(long double) == 16 rather than 8. GCC doesn't have such an option though, so avoiding that compiler option is probably preferable.

While this change is OK, we do want the UInt class to be trivially copyable. Do you know why it is not trivially copyable even when defaulted?

While this change is OK, we do want the UInt class to be trivially copyable. Do you know why it is not trivially copyable even when defaulted?

We also needed to default operator=, I'll update the patch with these changes.

@sivachandra while we don't need this anymore, I think it's a good addition to the codebase.

sivachandra added inline comments.Apr 20 2023, 9:11 AM
libc/src/__support/CPP/bit.h
32

I am on the fence about this. Should we be bit-casting instances of classes which are not trivially copyable? So, may be add something like this instead of trying to bit-cast:

static_assert(cpp::is_trivially_copyable<To>::value && cpp::is_trivially_copyable<From>::value,
                      "Cannot bit-cast instances of non-trivially copyable classes.");
libc/src/__support/CPP/bit.h
32

alright

BTW, in the possible implementation section of https://en.cppreference.com/w/cpp/numeric/bit_cast it also requires the dest type to be trivially constructible when using memcpy, should I add it too?

Added static assertions

sivachandra added inline comments.Apr 20 2023, 10:35 AM
libc/src/__support/CPP/bit.h
38

Shouldn't this be outside of the builtin check? Ideally, before the declaration of to on line 35?

sivachandra accepted this revision.Apr 20 2023, 10:36 AM
This revision is now accepted and ready to land.Apr 20 2023, 10:36 AM
sivachandra added inline comments.Apr 20 2023, 10:39 AM
libc/src/__support/CPP/type_traits.h
31

This is a libc internal file so we don't need the _ prefixes. Just use T here and below, and also just Args.

Addressed comments

This revision was landed with ongoing or failed builds.Apr 20 2023, 4:38 PM
This revision was automatically updated to reflect the committed changes.