[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 11/11] x86/bitops: Use the POPCNT instruction when available
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |