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

[Clint Caywood via Phabricator via libc-commits]

cratonica 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
----------------
lntue wrote:
> 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.
I'd like to hang onto it for a few reasons:

  - This approach is much different, so I wanted to require you to approve it again rather than sneaking in an entirely new approach under the radar
   - The diff between revisions would be messy, since the new code is much closer to HEAD than it is to where this branch (all new unit tests and the refactor to its own library would be deleted)
   - I'd like to be able to reference the original revision in the future. 

That being said, I don't want to deviate from the norms used by LLVM, so I can amend it here if you'd prefer.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117584