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

[Tue Ly via Phabricator via libc-commits]

lntue 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
----------------
sivachandra wrote:
> 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.
If using this pattern then the output type for unsigned int should be kept as unsigned int, and I agree that we should use sizeof(...)*8 - 1 instead of hard-coded values.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117684