[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 9/9] x86/bitops: Use the POPCNT instruction when available
On 27.08.2024 16:59, Andrew Cooper wrote: > On 27/08/2024 1:47 pm, Jan Beulich wrote: >> On 27.08.2024 13:17, Andrew Cooper wrote: >>> On 26/08/2024 2:07 pm, Jan Beulich wrote: >>>> On 23.08.2024 01:06, Andrew Cooper wrote: >>>>> @@ -475,4 +476,24 @@ static always_inline unsigned int arch_flsl(unsigned >>>>> long x) >>>>> } >>>>> #define arch_flsl arch_flsl >>>>> >>>>> +static always_inline unsigned int arch_hweightl(unsigned long x) >>>>> +{ >>>>> + unsigned int r; >>>>> + >>>>> + /* >>>>> + * arch_generic_hweightl() is written in ASM in order to preserve all >>>>> + * registers, as the compiler can't see the call. >>>>> + * >>>>> + * This limits the POPCNT instruction to using the same ABI as a >>>>> function >>>>> + * call (input in %rdi, output in %eax) but that's fine. >>>>> + */ >>>>> + alternative_io("call arch_generic_hweightl", >>>>> + "popcnt %[val], %q[res]", X86_FEATURE_POPCNT, >>>>> + ASM_OUTPUT2([res] "=a" (r) ASM_CALL_CONSTRAINT), >>>>> + [val] "D" (x)); >>>> If you made [val] an output ("+D") you could avoid preserving the register >>>> in the function. And I'd expect the register wouldn't be re-used often >>>> afterwards, so its clobbering likely won't harm code quality very much. >>> "+D" means it's modified by the asm, which forces preservation of the >>> register, if it's still needed afterwards. >>> >>> Plain "D" means not modified by the asm, which means it can be reused if >>> necessary. >> And we likely would prefer the former: If the register's value isn't >> use afterwards (and that's the case as far as the function on its own >> goes), the compiler will know it doesn't need to preserve anything. >> That way, rather than always preserving (in the called function) >> preservation will be limited to just the (likely few) cases where the >> value actually is still needed afterwards. > > Constraints are there to describe how the asm() behaves to the compiler. > > You appear to be asking me to put in explicitly-incorrect constraints > because you think it will game the optimiser. > > Except the reasoning is backwards. The only thing forcing "+D" will do > is cause the compiler to preserve the value elsewhere if it's actually > needed later, despite the contents of %rdi still being good for the purpose. > > In other words, using "+D" for asm which is really only "D" (as this one > is) will result in strictly worse code generation in the corner case you > seem to be worried about. Well, then leave it as is. (An extra benefit would have been that arch_generic_hweightl() then would ended up with a odd-number-of-slots stack frame. Not that this matters much, but having ABI-compliant functions where possible seems always preferable, when possible.) Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |