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

[Clint Caywood via Phabricator via libc-commits]

cratonica marked 4 inline comments as done.
cratonica added a comment.

Thanks, PTAL



================
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:
> 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.
Yes, sorry for that oversight. I originally used the uintxx_t values until I pivoted to using the non-aliased types, but forgot to drop the assumptions about bit width after that change.


================
Comment at: libc/src/__support/FPUtil/Hypot.h:35
+  if (mant > 0) {
+    shift_length = 31 - __builtin_clz(mant);
   }
----------------
sivachandra wrote:
> 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`.
Note that this assumes CHAR_BIT is 8, but I can't #include <climits> according to the lint rules.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117684