[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:
> 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.
> 
It's totally fine to me to have the a separate review patch, especially when you want to refer back to the original one in the future.  Just a few comments about how Phabricator works as I know of:
- If there is any update to the patch, I'd need to re-approve again (you will see that any accepted tick next reviewers will be changed from green to gray)
- If you amend the commit, Phabricator will show the diff between the latest change and HEAD by default.  But it still records all of your previous revisions (each time you run arc diff --update), and we can selectively diff between them if we want to.
- But yes, the default workflow from LLVM is a bit different from normal git workflow, where they prefer to have a single commit for each patch, and having their code review tools revolve around that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117584