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

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: llvm/include/llvm/ADT/bit.h:88
+#if __has_builtin(bswap64)
+    return __builtin_bswap64(UV);
+#elif defined(_MSC_VER) && !defined(_DEBUG)
----------------
Shouldn't this be `__has_builtin(__builtin_bswap64)` ?


================
Comment at: llvm/include/llvm/ADT/bit.h:70
+    uint32_t UV = V;
+#if defined(__llvm__) || (defined(__GNUC__) && !defined(__ICC))
+    return __builtin_bswap32(UV);
----------------
kazu wrote:
> barannikov88 wrote:
> > kazu wrote:
> > > kazu wrote:
> > > > 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.
> > > I've removed the dependence on GCC-style `__builtin_xxx`, so I don't use `__has_builtin`,
> > > 
> > > Both gcc and clang generate good code.
> > > According to compiler explorer, gcc-4.5.3 and clang-10.0.0 generate bswap for the following code snippet
> > They may generate the intrinsic for this sample code, but they also may fail to do so when this function is inlined.
> > I'd prefer seeing the intrinsic.
> > 
> OK.  I've restored the calls to intrinsics.
Shouldn't this be `__has_builtin(__builtin_bswap32)` ?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142274