Re: [Xen-devel] [PATCH 4/4] x86: use POPCNT for hweight<N>() when available

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.06.19 at 21:07, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 03/06/2019 09:13, Jan Beulich wrote:
>>>>> On 31.05.19 at 22:43, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 31/05/2019 02:54, Jan Beulich wrote:
>>>> @@ -245,6 +246,9 @@ boot/mkelf32: boot/mkelf32.c
>>>>  efi/mkreloc: efi/mkreloc.c
>>>>    $(HOSTCC) $(HOSTCFLAGS) -g -o $@ $<
>>>>  
>>>> +nocov-y += hweight.o
>>> Irrespective of the exact specifics of how the patch ends up, I don't
>>> think the nocov restriction is a direction we want to take.
>>>
>>> Coverage may not be a thing used in production, but when it is used for
>>> development, it needs to not have random holes missing in the results data.
>> Sure, but then we can't avoid saving/restoring the callee clobbered
>> registers in the to be called functions.
> 
> Why is this of concern?
> 
> a) it the compilers job to DTRT, and the sum total of GCC's coverage
> data appears to be "addq $1, mumble(%rip)"
> 
> b) coverage is just one of several things which might add
> instrumentation, ubsan being the other example which Xen already supports.

Hmm, have I been under the wrong impression that, like for -p, a
function call _might_ get inserted at the beginning of functions? Is
it spelled out somewhere what exact code can be inserted? The
command line option descriptions alone don't ... What we need to
avoid are function calls; I believe any other sort of code insertion
ought to be okay.

However - having seen your suggestion to follow Linux and use
assembly implementations: How would not adding coverage
instrumentation here differ from there not being any in assembly
code?

As to ubsan - yes, this should be treated the same way as the
coverage instrumentation. I.e. unless there's a guarantee that
it can't result in emitted function calls, it would need to be
suppressed.

In fact I was considering to add a build step to verify that the
object file has no external dependencies, i.e. in particular
doesn't call any functions.

>>  Which in turn means I see no
>> way of avoiding code duplications (be it in C or assembly) of the
>> generic_hweight<N>() implementations.
>>
>>>> +hweight.o: CFLAGS += $(foreach reg,cx dx si 8 9 10 11,-ffixed-r$(reg))
>>>> +
>>> Does this work with Clang?
>> I have no idea.
> 
> I do - every time I have a suspicion, the answer is invariably no.
> 
> clang: error: unknown argument: '-ffixed-rcx'
> clang: error: unknown argument: '-ffixed-rdx'
> clang: error: unknown argument: '-ffixed-rsi'
> clang: error: unknown argument '-ffixed-r8', did you mean '-ffixed-r9'?
> clang: error: unknown argument '-ffixed-r10', did you mean '-ffixed-r19'?
> clang: error: unknown argument '-ffixed-r11', did you mean '-ffixed-r19'?
> 
> As it turns out, the missing -ffixed-r9 is a Hexagon option only, and
> isn't applicable for x86.

And -ffixed-r19 hardly can be x86 either. Do they expect the 32-bit
register names? https://clang.llvm.org/docs/UsersManual.html has
no hit of -ffixed-* at all, despite the hints the compiler gives according
to the output you've quoted.

I have to admit that I find it pretty frustrating to have to deal with
such issues: If they claim to be gcc compatible, there shouldn't be
a need to think about using basically every not entirely standard
command line option. However much I like being able to have code
compilable with multiple compilers (not the least because one may
spot issues another doesn't), I don't think this is a basis to work
from. I.e. without a sufficient level of compatibility on their part
I'd like to raise the question of whether we really need to retain
clang compatibility in our code.

>>>> --- /dev/null
>>>> +++ b/xen/arch/x86/hweight.c
>>>> @@ -0,0 +1,28 @@
>>>> +#define generic_hweight64 _hweight64
>>>> +#define generic_hweight32 _hweight32
>>>> +#define generic_hweight16 _hweight16
>>>> +#define generic_hweight8  _hweight8
>>>> +
>>>> +#include <xen/compiler.h>
>>>> +
>>>> +#undef inline
>>>> +#define inline always_inline
>>>> +
>>>> +#include <xen/bitops.h>
>>>> +
>>>> +#undef generic_hweight8
>>>> +#undef generic_hweight16
>>>> +#undef generic_hweight32
>>>> +#undef generic_hweight64
>>>> +
>>>> +#define HWEIGHT(n)                                             \
>>>> +typeof(_hweight##n) generic_hweight##n;                        \
>>>> +unsigned int generic_hweight##n(typeof((uint##n##_t)0 + 0U) x) \
>>> A question to the rest of xen-devel.  Is there anyone else who can
>>> actually work out what this construct is doing?
>>>
>>> I'd like to get a feel for how many people can even follow some of our C.
>> I know you don't like such constructs, but you likely also know
>> that I don't like the redundancy resulting when not using them.
>> You've vetoed a change by Roger in this direction recently.
>> While I did accept this (as the code we have is fine as is as
>> well), I don't think your personal taste should rule out such
>> uses. If anything, may I ask for clear guidelines (to be put into
>> ./CODING_STYLE after having reached consensus) which parts
>> of the C language are fine to use, and which ones aren't?
> 
> If it were up to me, I'd reject any use of constructs like this.  They
> are fundamentally incompatible with easy-to-follow code, and I value
> that as a far more important property than the source file being a few
> lines shorter.
> 
> Particularly in this case, it really is just obfuscation, because the
> longhand version can be written shorter than the HWEIGHT macro alone,
> let alone its invocations:
> 
> unsigned int generic_hweight8 (unsigned int x) { return _hweight8(x);  }
> unsigned int generic_hweight16(unsigned int x) { return _hweight16(x); }
> unsigned int generic_hweight32(unsigned int x) { return _hweight32(x); }
> unsigned int generic_hweight64(uint64_t x)     { return _hweight64(x); }
> 
> is a far easier to read result.

Okay, I'll switch. But you realize that this loses a property I had
specifically added: My original variant also ensured _hweight<N>()
and generic_hweight<N>() have matching types.

>  That said, the point is moot because this file still doesn't compile.

With clang you mean? Or with gcc (where it obviously does compile
for me, so I'd appreciate more detail).

>>>> --- a/xen/include/asm-x86/bitops.h
>>>> +++ b/xen/include/asm-x86/bitops.h
>>>> @@ -469,15 +469,35 @@ static inline int fls(unsigned int x)
>>>>      return r + 1;
>>>>  }
>>>>  
>>>> +/* POPCNT encodings with %{r,e}di input and %{r,e}ax output: */
>>>> +#define POPCNT_64 ".byte 0xF3, 0x48, 0x0F, 0xB8, 0xC7"
>>>> +#define POPCNT_32 ".byte 0xF3, 0x0F, 0xB8, 0xC7"
>>> So (the dangers of false micro-optimsiation aside), POPCNT_32 will
>>> probably be better using a redundant %ds prefix.
>> For the use in hweight32() - perhaps. But not for the uses in
>> hweight{16,8}(), as there original code and replacement fully
>> match up in lengths.
> 
> Looking at the compiler generated code, the 32 and 16 bit are identical
> other than the final imul.  The 8 bit case is very similar, but can
> actually get away with imm8 rather than imm32.

What imul? What imm8 / imm32? Are you taking about
generic_hweight<N>()? The code fragment above is the to-be-
patched-in code, which shouldn't require anything like this.

> There is one caller each for the 8 and 16 bit version, both of which are
> in is_multicast_dest() in the vlapic emulation code.  This particular
> example would actually benefit from Linux's example of aliasing of the 8
> and 16bit versions to the 32bit version.

Possibly. However, for one I don't think specific use sites should
influence the decision here. And then I don't think hweight() of
any form is needed in those two cases in the first place. All they
want to know is whether more than one bit is set, which can be
achieved by a standard is-power-of-2 check.

> The only user which is in any way a fastpath is in __bitmap_weight(),
> and I've got another cleanup patch for that.  (I also bet that doing an
> x86 specific vectorised version of __bitmap_weight() using POPCNT's
> memory operand would be a perf win, but that can wait until some free
> and some proper profiling support appears.)
> 
> Given that this is a helper which is unlikely to ever be rewritten, and
> there doesn't appear to be an obvious way to get Clang to avoid using
> specific registers, I'd still recommend the Linux way, with the 32 and
> 64bit versions custom written in assembly, and not worry about the
> duplication.

While you're likely right about the "never be rewritten" part (leaving
aside the logic change in patch 1 of the series, which - had we had
an assembly implementation already - would have meant a rewrite),
I'm still rather puzzled by you suggesting to extend the amount of
assembly code we have, when it can be easily written in C.

Anyway, if I was to clone Linux'es code, I wouldn't use it entirely
as is anyway (in particular I'd like to stick to clobbering %rdi at
the call site, rather than preserving it in the functions). Yet you
seem to be demanding us doing so.

> Talking of binutils, when are we going to get around to upping our
> minimum version?

I guess as soon as someone cares enough to submit a proposal of
a new minimum version that would cover all environments we care
about to be able to build on. Iirc there's some breakage already
anyway for the oldest version we claim to support.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel