Date: Mon, 4 Dec 2017 14:43:19 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Allan Jude <allanjude@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r326506 - in head/sys/contrib/zstd/lib: common compress Message-ID: <20171204134316.O1755@besplex.bde.org> In-Reply-To: <201712040116.vB41GQ41078524@repo.freebsd.org> References: <201712040116.vB41GQ41078524@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 4 Dec 2017, Allan Jude wrote:
> Log:
> Use __has_builtin() to ensure clz and clzll builtins are available
>
> The existing check of the GCC version number is not sufficient
It also checked a wrong version number, and still doesn't.
> This fixes the build on sparc64 in preparation for integrating ZSTD into
> the kernel for ZFS and Crash Dumps.
This doesn't really work. __has_builtin() is a dummy (always 0) in
at least gcc-4.2.1 and earlier versions of gcc, and is broken as
designed in compilers that implement it. It just tells you if the
compiler supports the builtin, but you don't want to know about the
builtin unless the builtin will generate better code for the target
arch than your alternative. Using __has_builtin() is especially wrong
in the kernel and other pure freestanding cases. Then the builtin
generates a libcall to nonexistent function unless the target arch
supports a better inline alternative. Nonexistent functions are never
better alternatives than yours.
> Modified: head/sys/contrib/zstd/lib/common/bitstream.h
> ==============================================================================
> --- head/sys/contrib/zstd/lib/common/bitstream.h Mon Dec 4 01:14:17 2017 (r326505)
> +++ head/sys/contrib/zstd/lib/common/bitstream.h Mon Dec 4 01:16:26 2017 (r326506)
> @@ -175,7 +175,7 @@ MEM_STATIC unsigned BIT_highbit32 (register U32 val)
> unsigned long r=0;
> _BitScanReverse ( &r, val );
> return (unsigned) r;
> -# elif defined(__GNUC__) && (__GNUC__ >= 3) /* Use GCC Intrinsic */
This version check is very broken. gcc-3.3.3 doesn't have __builtin_clz(),
but passes the check.
> +# elif defined(__GNUC__) && (__GNUC__ >= 3) && __has_builtin(__builtin_clz) /* Use GCC Intrinsic */
gcc-4.2.1 doesn't have __builtin_clz() on x86, but not __has_builtin().
sys/cdefs.h defines __has_builtin() as 0 unless __has_builtin is defined.
The first part of the test is now usually just an obfuscation. It
still uses a wrong version number and a redumdant test that __GNUC__
is defined, but when (__GNUC__ < 3), __has_builtin() is usually 0
and gives the same result of 0. It is only in the unusual (unsupported)
case where the compiler doesn't pretend to be gcc but defines __has_builtin()
that the first part of the check has an effect, and then its effect is to
break the second part.
sys/cdefs.h gives an example of a similar but presumably-correct version
check. For using __builtin_unreachable(), it doesn't trust __has_builtin(),
but checks that the version is >= 4.6. 4.6 is fine-grained enough to have
a chance of being correct.
> return 31 - __builtin_clz (val);
I think this is only for userland, so it is not broken when the builtin
reduce a libcall. The libcall is presumably to __clzdi2(). The kernel
doesn't have this, but libgcc.a does.
> # else /* Software version */
> static const unsigned DeBruijnClz[32] = { 0, 9, 1, 10, 13, 21, 2, 29,
This is unreachable when the libcall is used.
If optimizing this is actually important, then so not is pessimizing it on
arches with the builtin implemented in hardware but __has_builtin() not
implemented.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20171204134316.O1755>