svn commit: r280279 - head/sys/sys

[Konstantin Belousov]

On Sat, Mar 21, 2015 at 09:33:22PM +1100, Bruce Evans wrote:
> On Sat, 21 Mar 2015, Konstantin Belousov wrote:
> How does the unconditional use of popcntq (in inline asm that is called
> from amd64 pmap) even work?
It is not unconditional.  The pmap code does the following:

		if ((cpu_feature2 & CPUID2_POPCNT) == 0) {
			free = popcnt_pc_map_elem(pc->pc_map[0]);
			...
		} else {
			free = popcntq(pc->pc_map[0]);
			...
		}

> 
> jhb's cleanups apparently missed this inline asm, while previous work
> should not have added it.  This inline asm should never have existed,
> since compilers can generate the same code with less-incorrect
> configuration.  Correct configuration would be messy.  jhb's cleanups
> depend on a compiler predefine (__POPCNT__) to determine if the
> instruction can be used.  But this definition is static and is usually
> wrong, since compilers default to plain amd64 which doesn't have the
> instruction.  Dynamic configuration would have to use cpuid just to
> determine if the instruction exists.
Exactly, this is what supposed to be done, and is done in the pmap.
It is fine for somebody targeting specific machine to change -march,
but it cannot be used to optimize the generic kernel which is shipped.

> 
> >>> Jilles noted that gcc head and 4.9.2 already provides a workaround by
> >>> xoring the dst register.  I have some patch for amd64 pmap, see the end
> >>> of the message.
> 
> I thought that that couldn't work since it uses the instruction
> unconditionally.  But the old code already does that.
> 
> >> IIRC, then patch never never uses asm, but intentionally uses the popcount
> >> builtin to avoid complications.
> 
> popcntq() in amd64/include/cpufunc.h does use asm, but is missing the
> complications needed for when it the instruction is not available.
> All callers (just 3 in pmap) seem to be broken, since they are also
> missing the complications.
See above.

> > Still, it is weird to provide functions from the sys/types.h namespace,
> > and even more weird to provide such special-purpose function.
> 
> Not in the sys/types.h, but in the implementation namespace :-).  Since
> no one should depend on the implementation details, we can move the
> details to a better place when one is known.  Better places would be
> something like libkern for userland to hold a large collection of
> functions, and a Standard place for single functions.  I expect the
> Standard place will be broken as designed for compatibility.  Probably
> strings.h alongside ffs().
strings.h sounds less surprising than sys/types.h

> 
> The special (inline/asm) functions are in sys/types.h on amd64 are
> currently limited to just 3 bswap functions and some new popcnt functions.