From: Nathan Chancellor <nathan@kernel.org>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: llvm@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: Adding __popcountsi2 and __popcountdi2
Date: Thu, 24 Apr 2025 21:11:38 -0500 [thread overview]
Message-ID: <20250425021138.GA3800209@ax162> (raw)
In-Reply-To: <CAHk-=whfT3A8K2Z+WbieGG5Hhc9QAT5s3qsbB19O0Roj2G5tfA@mail.gmail.com>
On Thu, Apr 24, 2025 at 06:36:33PM -0700, Linus Torvalds wrote:
> On Thu, 24 Apr 2025 at 17:33, Nathan Chancellor <nathan@kernel.org> wrote:
> >
> > I figured added
> > these may not be as bad as the wcslen() case because most architectures
> > generally have an optimized popcount implementation and I am not sure
> > compiler builtins are banned entirely from the kernel but I can
> > understand if it is still contentious.
>
> Why does the compiler even bother to do this if the architecture
> doesn't have the popcount instruction? The function call is quite
> possibly more expensive than just doing it the stupid way.
Not entirely sure what the motivation is from the compiler side but I
cannot immagine that they would be doing this if it was not more
efficient in some way.
> But if you want to do this, put the damn thing as an alias on the code
> that actually *does* the SW fallback in lib/hweight.c.
>
> Because the way your patch does it now, it takes "I'm doing stupid
> things" to the next level by turning that function call into *two*
> function calls - first calling __popcountsi2, which then calls
> __sw_hweight32.
>
> Let's not do stupid things, ok?
I will test declaring __popcount{s,d}i2() as aliases of
__sw_hweight{32,64}() to see what effect that has but I figured that
calling the __arch_hweight variants was more correct because some
architectures (at least RISC-V and x86 when I looked) use alternatives
in that path to use hardware instructions and avoid the software path
altogether. While there would still be the overhead from the function
call, I figured not using the software fallback would at least soften
that blow.
Cheers,
Nathan
next prev parent reply other threads:[~2025-04-25 2:11 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-04-25 0:33 Adding __popcountsi2 and __popcountdi2 Nathan Chancellor
2025-04-25 1:36 ` Linus Torvalds
2025-04-25 2:11 ` Nathan Chancellor [this message]
2025-04-25 3:30 ` Linus Torvalds
2025-05-05 15:05 ` Maciej W. Rozycki
2025-05-05 16:03 ` Linus Torvalds
2025-05-05 23:13 ` Maciej W. Rozycki
2025-04-30 12:12 ` David Laight
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250425021138.GA3800209@ax162 \
--to=nathan@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=llvm@lists.linux.dev \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.