[ADT] llvm::bit_cast - use __builtin_bit_cast if available

[RKSimon]

Summary

If the compiler supports __builtin_bit_cast we should try to use it instead of std::memcpy (and avoid including the cstring header).

[barannikov88]

LGTM

[dblaikie]

If the compiler supports __builtin_bit_cast we should try to use it instead of std::memcpy (and avoid including the cstring header).

Why? Maintaining non-portable code has some cost, if we can get similar/sufficient/the same performance without special casing, that seems better. Do we not get adequate performance with std::memcpy? Is the header especially expensive to include?

[RKSimon]

In D142305#4077361, @dblaikie wrote:

If the compiler supports __builtin_bit_cast we should try to use it instead of std::memcpy (and avoid including the cstring header).

Why? Maintaining non-portable code has some cost, if we can get similar/sufficient/the same performance without special casing, that seems better. Do we not get adequate performance with std::memcpy? Is the header especially expensive to include?

No strong preference tbh

[barannikov88]

In D142305#4077361, @dblaikie wrote:

Why? Maintaining non-portable code has some cost, if we can get similar/sufficient/the same performance without special casing, that seems better. Do we not get adequate performance with std::memcpy? Is the header especially expensive to include?

Half a point is that __builtin_bit_cast can be used in constant expressions. "Half", because llvm::bit_cast is not constexpr :)

[dblaikie]

In D142305#4077555, @RKSimon wrote:

In D142305#4077361, @dblaikie wrote:

If the compiler supports __builtin_bit_cast we should try to use it instead of std::memcpy (and avoid including the cstring header).

Why? Maintaining non-portable code has some cost, if we can get similar/sufficient/the same performance without special casing, that seems better. Do we not get adequate performance with std::memcpy? Is the header especially expensive to include?

No strong preference tbh

Did something motivate proposing this change?

[RKSimon]

In D142305#4078205, @dblaikie wrote:

In D142305#4077555, @RKSimon wrote:

In D142305#4077361, @dblaikie wrote:

If the compiler supports __builtin_bit_cast we should try to use it instead of std::memcpy (and avoid including the cstring header).

Why? Maintaining non-portable code has some cost, if we can get similar/sufficient/the same performance without special casing, that seems better. Do we not get adequate performance with std::memcpy? Is the header especially expensive to include?

No strong preference tbh

Did something motivate proposing this change?

This was part of a general attempt to get our bit.h as close to c++ <bit> as possible - especially when I realised that all latest supported compilers already implement '__builtin_bit_cast' : https://godbolt.org/z/YWM4GcceE

[dblaikie]

In D142305#4082091, @RKSimon wrote:

In D142305#4078205, @dblaikie wrote:

In D142305#4077555, @RKSimon wrote:

In D142305#4077361, @dblaikie wrote:

If the compiler supports __builtin_bit_cast we should try to use it instead of std::memcpy (and avoid including the cstring header).

Why? Maintaining non-portable code has some cost, if we can get similar/sufficient/the same performance without special casing, that seems better. Do we not get adequate performance with std::memcpy? Is the header especially expensive to include?

No strong preference tbh

Did something motivate proposing this change?

This was part of a general attempt to get our bit.h as close to c++ <bit> as possible - especially when I realised that all latest supported compilers already implement '__builtin_bit_cast' : https://godbolt.org/z/YWM4GcceE

I'd say unless there's some significant gain - maintaining #ifdef'd non-portable(to all supported compilers) codepaths isn't worth the maintenance burden. But I don't feel strongly enough to insist on a revert - up to you.

[mib]

Hey @RKSimon, it seems this patch is cause lldb's macOS ASAN bot to fail. Could you take a look ?

/Users/buildslave/jenkins/workspace/lldb-cmake-sanitized/llvm-project/llvm/include/llvm/ADT/bit.h:55:3: error: non-void function 'bit_cast' should return a value [-Wreturn-type]
  return __builtin_bit_cast(To, from);
  ^

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-sanitized/3475/console

llvm/include/llvm/ADT/bit.h

 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_ADT_BIT_H
 #define LLVM_ADT_BIT_H
 
 #include "llvm/Support/Compiler.h"
 #include <cstdint>
-#include <cstring>
 #include <limits>
 #include <type_traits>
 
+#if !__has_builtin(__builtin_bit_cast)
+#include <cstring>
+#endif
+
 #if defined(_MSC_VER) && !defined(_DEBUG)
 #include <cstdlib>  // for _byteswap_{ushort,ulong,uint64}
 #endif
 
 #ifdef _MSC_VER
 // Declare these intrinsics manually rather including intrin.h. It's very
 // expensive, and bit.h is popular via MathExtras.h.
 // #include <intrin.h>

 //  - It requires trivially-constructible To, to avoid UB in the implementation.
 template <
     typename To, typename From,
     typename = std::enable_if_t<sizeof(To) == sizeof(From)>,
     typename = std::enable_if_t<std::is_trivially_constructible<To>::value>,
     typename = std::enable_if_t<std::is_trivially_copyable<To>::value>,
     typename = std::enable_if_t<std::is_trivially_copyable<From>::value>>
 [[nodiscard]] inline To bit_cast(const From &from) noexcept {
+#if __has_builtin(__builtin_bit_cast)
+  return __builtin_bit_cast(To, from);
+#else
   To to;
   std::memcpy(&to, &from, sizeof(To));
   return to;
+#endif
 }
 
 /// Reverses the bytes in the given integer value V.
 template <typename T, typename = std::enable_if_t<std::is_integral_v<T>>>
 [[nodiscard]] constexpr T byteswap(T V) noexcept {
   if constexpr (sizeof(T) == 1) {
     return V;
   } else if constexpr (sizeof(T) == 2) {