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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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:
>>>> A few RFC points.
>>>>
>>>>  * I throught we had an x86 general lib-y but I can't find one, hence why 
>>>> it's
>>>>    still in xen/lib/ for now.
>>> We indeed have nothing like that yet. The file name should then imo not be
>>> arch-* though, but x86-*. Or you could put it in xen/lib/x86/.
>> I was worried about xen/lib/x86/ and interfering with userspace.
>>
>>> It could even
>>> be obj-y rather than lib-y, unless you expect we'll be able to get rid of
>>> all uses, which seems unlikely at least due to bitmap_weight(). Otoh with
>>> my ABI-level series the call site in arch_hweightl() could then be made go
>>> away for v2 and above, at which point it living in lib-y will be preferable.
>> Yes, I was specifically trying to account for this.
>>
>> I'm expecting the mandatory-popcnt work to end up looking something like:
>>
>> diff --git a/xen/arch/x86/include/asm/bitops.h
>> b/xen/arch/x86/include/asm/bitops.h
>> index 0db698ed3f4c..027eda60c094 100644
>> --- a/xen/arch/x86/include/asm/bitops.h
>> +++ b/xen/arch/x86/include/asm/bitops.h
>> @@ -480,6 +480,9 @@ static always_inline unsigned int
>> arch_hweightl(unsigned long x)
>>  {
>>      unsigned int r;
>>  
>> +    if ( IS_ENABLED(CONFIG_REQUIRE_POPCNT /* or whatever */) )
>> +        return __builtin_popcountl(x);
>> +
>>      /*
>>       * arch_generic_hweightl() is written in ASM in order to preserve all
>>       * registers, as the compiler can't see the call.
>>
>>
>> which in turn DCE's the alternative_io() and drops the reference to
>> arch_generic_hweightl().
> Right, that's along the lines of what I was thinking to re-base to once
> your work has gone in. (I have close to zero hope that my work would be
> going in first. [1]) Just that I don't think we'll have separate
> CONFIG_REQUIRE_<feature> settings. Yet how exactly that wants structuring
> is something we ought to discuss there, not here.

I just couldn't remember the name you'd given the option.  I wasn't
trying to driveby review it here.

>
>>>>  * When we up the minimum toolchain to GCC 7 / Clang 5, we can use a
>>>>    __attribute__((no_caller_saved_registers)) and can forgo writing this 
>>>> in asm.
>>>>
>>>>    GCC seems to need extra help, and wants -mgeneral-regs-only too.  It 
>>>> has a
>>>>    habit of complaining about incompatible instructions even when it's not
>>>>    emitting them.
>>> What is this part about? What incompatible instructions, in particular?
>> This was weird.  https://godbolt.org/z/4z1qzWbfE is an example.
> That's because apparently in your experiments you didn't add -mno-sse. If you
> incrementally add that, then -mno-mmx, then -msoft-float, you'll first see the
> diagnostic change and then observe it to finally compile. And yes, from
> looking at the gcc code emitting this error, this is solely tied to the ISAs
> enabled at the time the function is being compiled. It's independent of the
> choice of insns. Pretty clearly a shortcoming, imo.

Ah - it was -msoft-float which I was looking for.

I'd played around with -mno-{sse,mmx} but not gotten a working result.


>
>>>> @@ -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.

~Andrew