Re: [PATCH v2 11/11] x86/bitops: Use the POPCNT instruction when available

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/09/2024 9:06 am, Jan Beulich wrote:
> On 30.08.2024 22:03, Andrew Cooper wrote:
>> On 29/08/2024 3:36 pm, Jan Beulich wrote:
>>> On 29.08.2024 00:03, Andrew Cooper wrote:
>>>> It has existed in x86 CPUs since 2008, so we're only 16 years late adding
>>>> support.  With all the other scafolding in place, implement arch_hweightl()
>>>> for x86.
>>>>
>>>> The only complication is that the call to arch_generic_hweightl() is behind
>>>> the compilers back.  Address this by writing it in ASM and ensure that it
>>>> preserves all registers.
>>>>
>>>> Copy the code generation from generic_hweightl().  It's not a complicated
>>>> algorithm, and is easy to regenerate if needs be, but cover it with the 
>>>> same
>>>> unit tests as test_generic_hweightl() just for piece of mind.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>>>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>>>
>>>> v2:
>>>>  * Fix MISRA 8.2 (parameter name) and 8.5 (single declaration) regressions.
>>>>  * Rename {arch->x86}-generic-hweightl.{S->c}
>>>>  * Adjust ASM formating
>>>>
>>>> The __constructor trick to cause any reference to $foo() to pull in
>>>> test_$foo() only works when both are in the same TU.  i.e. what I did in
>>>> v1 (putting test_arch_generic_hweightl() in the regular generic-hweightl.c)
>>>> didn't work.
>>> I'm afraid I don't understand this. What exactly didn't work, breaking in 
>>> which
>>> way? Presumably as much as you, I don't really like the global asm() in a C
>>> file, when ideally the same could be written with less clutter in an 
>>> assembly
>>> one.
>> We have lib-y because we wish to not include arch_generic_hweightl() if
>> it isn't referenced.
>>
>> But, test_arch_generic_hweightl() unconditionally references
>> arch_generic_hweightl() (in CONFIG_SELF_TESTS builds).
> Which I assumed was intentional: Always have the tests, to make sure the code
> doesn't go stale (or even break).

Come to think of it, we already have two subtlety different things.

common/bitops.c is SELF_TESTS only, and does both compile time
(constant-folding) and runtime (non-folded) tests on the top-level APIs.

This checks that the compiler agrees with the test parameters that I (or
hopefully in the future, others) have chosen, and that every arch_$foo()
override agrees with the compiler version.

It has already found bugs.  Both __builtin foo() vs fool() mismatches
and I dread to think how long a bugs like that could have gone
unnoticed.  I caught ppc's arch_hweightl() before posting v1, but noone
noticed ARM's arch_flsl() snafu when it was on the list (I found it when
retrofitting SELF_TESTS around the series).


Separately, anything that causes any {arch_,}generic_$foo()'s to be
included also pulls in the SELF_TEST for that specific function. 
They're runtime only tests (no constant folding in an intentionally
out-of-line function).

But, it was intentional to try and make CONFIG_SELF_TESTS not blindly
pull in the unused generic_$foo()'s.  They'll get tested if they are
referenced in a build, but the tests are only liable to break are during
bitops development or a new/weird arch/environments.

Furthermore, I expect (/hope) that we'll get to the point where we're
being conservative with selftests.  It's part of why I'm trying to keep
the current ones pretty lean.


> Looking at the v2 code again I notice that while now you're running the tests
> only when the to-be-tested function is referenced from elsewhere, the testing
> has become independent of CONFIG_SELF_TESTS. I don't think that was intended?

Correct - that was an oversight.  The CONFIG_SELF_TESTS guard wants to
go back in.

~Andrew