[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.

Please take a look.  Thanks!



================
Comment at: llvm/include/llvm/ADT/bit.h:70
+    uint32_t UV = V;
+#if defined(__llvm__) || (defined(__GNUC__) && !defined(__ICC))
+    return __builtin_bswap32(UV);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142274