From owner-svn-src-head@freebsd.org Mon Dec 4 03:43:29 2017 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 09FD7DEBDF2; Mon, 4 Dec 2017 03:43:29 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail109.syd.optusnet.com.au (mail109.syd.optusnet.com.au [211.29.132.80]) by mx1.freebsd.org (Postfix) with ESMTP id 85F08766D8; Mon, 4 Dec 2017 03:43:28 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail109.syd.optusnet.com.au (Postfix) with ESMTPS id 9E924D61AA7; Mon, 4 Dec 2017 14:43:19 +1100 (AEDT) Date: Mon, 4 Dec 2017 14:43:19 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Allan Jude 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 In-Reply-To: <201712040116.vB41GQ41078524@repo.freebsd.org> Message-ID: <20171204134316.O1755@besplex.bde.org> References: <201712040116.vB41GQ41078524@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=bc8baKHB c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=4F3b_KXgID9f6kN2jZ4A:9 a=j6TpNBZckAQal6ih:21 a=Y0zHFzzHz-yrQfB7:21 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Dec 2017 03:43:29 -0000 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