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

[Sergei Barannikov via Phabricator via llvm-commits]

barannikov88 added inline comments.


================
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:
> 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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142274