[libc-commits] [PATCH] D117584: [libc] Specialize portion of hypot for x86_64 CPUs.

[Tue Ly via Phabricator via libc-commits]

lntue added inline comments.


================
Comment at: libc/src/__support/x86_64/find_leading_one.h:28-31
+  __asm__("bsr %[value], %[value]"
+          : [value] "=r"(value)
+          : "0"(value)
+          : "flags"); // Clobbers ZF
----------------
cratonica wrote:
> sivachandra wrote:
> > cratonica wrote:
> > > sivachandra wrote:
> > > > It seems to me like one can use `__builtin_clz` and avoid target assembly? For example: https://godbolt.org/z/eY8n8P118
> > > Nice, that also works for ARM: https://godbolt.org/z/bY9nfEd16
> > > 
> > > Are we OK using such builtins in this library ? If so, should we get rid of the original find_leading_one() function altogether and just call __builtin_clz ? Or should it still be specialized for each architecture and delegate at that level?
> > Yes, it is OK to use compiler builtins for this use case. We can get rid of the original `find_leading_one`.
> Sent https://reviews.llvm.org/D117684
Normally you could just amend the commit and update the patch review with $ arc diff --update D.... instead of creating a new one, unless you want to keep the old review around.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117584