[libc-commits] [PATCH] D117684: [libc] Use __builtin_clz to find leading 1 in hypot

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/__support/FPUtil/Hypot.h:26
+// __builtin_clz* rather than using the exactly-sized aliases from stdint.h
+// (such as uint32_t). There are 3 overloads even though 2 will only ever be
+// used by a specific platform, since unsigned long varies in size depending on
----------------
lntue wrote:
> builtin_clzl is just extended to either builtin_clz or builtin_clzll depending on the size of unsigned long. It might be better to keep uint32_t and uint64_t specializations and directly use __builtin_clz for uint32_t, builtin_clzll for uint64_t, together with static_assert(sizeof(unsigned int) == 4) and static_assert(sizeof(unsigned long long) == 8) instead?
Considering that `uint32_t` and `uint64_t` are just type sugar and not actual types, the existing code seems to me like the best approach and keeps it really straighforward with no assumptions of any sizes.


================
Comment at: libc/src/__support/FPUtil/Hypot.h:35
+  if (mant > 0) {
+    shift_length = 31 - __builtin_clz(mant);
   }
----------------
Use a pattern like `sizeof(unsigned int) - 1` instead of hard coding here and below. For, in the `unsigned long` case, `63 - <...>`can be more than the width of `unsigned long`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117684