[PATCH] D142274: [ADT] Add llvm::byteswap to bit.h

[Kazu Hirata via Phabricator via llvm-commits]

kazu marked an inline comment as done.
kazu added a comment.

In D142274#4071228 <https://reviews.llvm.org/D142274#4071228>, @RKSimon wrote:

> Are you planning to update SwapByteOrder.h to use this?

Yes, and users of `getSwappedBytes` for integer types.



================
Comment at: llvm/include/llvm/ADT/bit.h:70
+    uint32_t UV = V;
+#if defined(__llvm__) || (defined(__GNUC__) && !defined(__ICC))
+    return __builtin_bswap32(UV);
----------------
RKSimon wrote:
> use has_builtin ?
Shall we stop using the intrinsics and just stick to the ANDs and ORs?  This way, we don't have to worry about the availability of intrinsics, the availability of has_builtin itself, whether we are using ICC, etc.  (I couldn't find out why ICC is excluded here.)  According to compiler explorer, gcc-4.5.3 and clang-10.0.0 generate bswap for the following code snippet:

```
#include <stdint.h>

uint32_t bs32(uint32_t v) { 
    uint32_t Byte0 = v & 0x000000FF;
    uint32_t Byte1 = v & 0x0000FF00;
    uint32_t Byte2 = v & 0x00FF0000;
    uint32_t Byte3 = v & 0xFF000000;
    return (Byte0 << 24) | (Byte1 << 8) | (Byte2 >> 8) | (Byte3 >> 24);
}

uint64_t bs64(uint64_t v) {
    uint64_t Hi = bs32(v);
    uint32_t Lo = bs32(v >> 32);
    return (Hi << 32) | Lo;
}
```

clang prior to 10.0.0 does not generate 64-bit bswap, but we don't attempt to use 64-bit bswap intrinsic in this patch or `SwapByteOrder.h` anyway.

Note that I mention gcc-4.5.3 above, but we require gcc-7.1.0.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142274