Re: [PATCH v2] x86: use POPCNT for hweight<N>() when available

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.03.2023 17:31, Roger Pau Monné wrote:
> On Tue, Mar 21, 2023 at 04:35:54PM +0100, Jan Beulich wrote:
>> On 21.03.2023 15:57, Roger Pau Monné wrote:
>>> On Mon, Mar 20, 2023 at 10:48:45AM +0100, Jan Beulich wrote:
>>>> On 17.03.2023 13:26, Andrew Cooper wrote:
>>>>> On 17/03/2023 11:22 am, Roger Pau Monné wrote:
>>>>>> On Mon, Jul 15, 2019 at 02:39:04PM +0000, Jan Beulich wrote:
>>>>>>> This is faster than using the software implementation, and the insn is
>>>>>>> available on all half-way recent hardware. Therefore convert
>>>>>>> generic_hweight<N>() to out-of-line functions (without affecting Arm)
>>>>>>> and use alternatives patching to replace the function calls.
>>>>>>>
>>>>>>> Note that the approach doesn#t work for clang, due to it not recognizing
>>>>>>> -ffixed-*.
>>>>>> I've been giving this a look, and I wonder if it would be fine to
>>>>>> simply push and pop the scratch registers in the 'call' path of the
>>>>>> alternative, as that won't require any specific compiler option.
>>>>
>>>> Hmm, ...
>>>>
>>>>> It's been a long while, and in that time I've learnt a lot more about
>>>>> performance, but my root objection to the approach taken here still
>>>>> stands - it is penalising the common case to optimise some pointless
>>>>> corner cases.
>>>>>
>>>>> Yes - on the call path, an extra push/pop pair (or few) to get temp
>>>>> registers is basically free.
>>>>
>>>> ... what is "a few"? We'd need to push/pop all call-clobbered registers
>>>> except %rax, i.e. a total of eight. I consider this too much. Unless,
>>>> as you suggest further down, we wrote the fallback in assembly. Which I
>>>> have to admit I'm surprised you propose when we strive to reduce the
>>>> amount of assembly we have to maintain.
>>>
>>> AMD added popcnt in 2007 and Intel in 2008.  While we shouldn't
>>> mandate popcnt support, I think we also shouldn't overly worry about
>>> the non-popcnt path.
>>
>> We agree here.
>>
>>> Also, how can you assert that the code generated without the scratch
>>> registers being usable won't be worse than the penalty of pushing and
>>> popping such registers on the stack and letting the routines use all
>>> registers freely?
>>
>> Irrespective of the -ffixed-* the compiler can make itself sufficiently
>> many registers available to use, by preserving just the ones it actually
>> uses. So that's pretty certainly not more PUSH/POP than when we would
>> blindly preserve all which may need preserving (no matter whether
>> they're actually used).
>>
>> Yet then both this question and ...
>>
>>> I very much prefer to have a non-optimal non-popcnt path, but have
>>> popcnt support for both gcc and clang, and without requiring any
>>> compiler options.
>>
>> ... this makes me wonder whether we're thinking of fundamentally
>> different generated code that we would end up with. Since the
>> (apparently agreed upon) goal is to optimize for the "popcnt
>> available" case, mainline code should be not significantly longer that
>> what's needed for the single instruction itself, or alternatively a
>> CALL insn. Adding pushes/pops of all call clobbered registers around
>> the CALL would grow mainline code too much (for my taste), i.e.
>> irrespective of these PUSH/POP all getting NOP-ed out by alternatives
>> patching. (As an aside I consider fiddling with the stack pointer in
>> inline asm() risky, and hence I would prefer to avoid such whenever
>> possible.
> 
> Is this because we are likely to not end up with a proper trace if we
> mess up with the stack pointer before a function call?

That's a related issue, but not what I was thinking about.

>  I would like
> to better understand your concerns with the stack pointer and inline
> asm().

You can't use local variables anymore with "m" or "rm" constraints, as
they might be accessed via %rsp.

> Other options would be using no_caller_saved_registers, but that's not
> available in all supported gcc versions, likely the same with clang,
> but I wouldn't mind upping the minimal clang version.

Right, you did suggest using this attribute before. But we continue to
support older gcc.

> What about allocating the size of the registers that need saving as an
> on-stack variable visible to the compiler and then moving to/from
> there in the inline asm()?

That would address the fiddling-with-%rsp concern, but would further
grow mainline code size.

>> Yes, it can be written so it's independent of what the
>> compiler thinks the stack pointer points at, but such constructs are
>> fragile as soon as one wants to change them a little later on.)
>>
>> Are you perhaps thinking of indeed having the PUSH/POP in mainline
>> code? Or some entirely different model?
>>
>> What I could see us doing to accommodate Clang is to have wrappers
>> around the actual functions which do as you suggest and preserve all
>> relevant registers, no matter whether they're used. That'll still be
>> assembly code to maintain, but imo less of a concern than in Andrew's
>> model of writing hweight<N>() themselves in assembly (provided I
>> understood him correctly), for being purely mechanical operations.
> 
> If possible I would prefer to use the same model for both gcc and
> clang, or else things tend to get more complicated than strictly
> needed.

We can always decide to accept the extra overhead even with gcc.

> I also wonder whether we could also get away without disabling of
> coverage and ubsan for the hweight object file.  But I assume as long
> ass we do the function call in inline asm() (and thus kind of hidden
> from the compiler) we need to disable any instrumentation because the
> changed function context.  I don't think there's anyway we can
> manually add the function call prefix/suffix in the inline asm()?

I don't know whether that would be possible (and portable across
versions). But what I'm more concerned about is that such functions
may also end up clobbering certain call-clobbered registers. (Note
that when writing these in assembly, as suggested by Andrew, no such
hooks would be used either.)

Jan