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
