From owner-svn-src-all@freebsd.org Tue Nov 28 21:56:03 2017 Return-Path: Delivered-To: svn-src-all@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 882A2DF009D; Tue, 28 Nov 2017 21:56:03 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail107.syd.optusnet.com.au (mail107.syd.optusnet.com.au [211.29.132.53]) by mx1.freebsd.org (Postfix) with ESMTP id DF84E7A5CF; Tue, 28 Nov 2017 21:56:02 +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 mail107.syd.optusnet.com.au (Postfix) with ESMTPS id EE9DDD46CA6; Wed, 29 Nov 2017 08:55:55 +1100 (AEDT) Date: Wed, 29 Nov 2017 08:55:55 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Alex Richardson cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r326342 - head/lib/libc/mips/gen In-Reply-To: <201711282037.vASKbRIj068414@repo.freebsd.org> Message-ID: <20171129074249.B1145@besplex.bde.org> References: <201711282037.vASKbRIj068414@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=cK6QihWN c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=oa1cjdcb5Po7lzyVjUwA:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 28 Nov 2017 21:56:03 -0000 On Tue, 28 Nov 2017, Alex Richardson wrote: > Log: > Fix fabs() for MIPS when used on -0.0 Only arm and mips have this in C in libc. arm still has the broken version, libm has better versions. Only 1 in C and none in asm, where libc has many C and asm versions with at least 1 still wrong. The version in libc is almost never used, ust like the versions in libc, since the builtin versions are usually used automatically. > It would previously return negative zero for -0.0 since -0.0 does not > compare less than 0. The issue was discovered when running the libc++ > test suite on softfloat MIPS64. Since the builtin is usually used, test programs usually don't find this bug. Presumably softfloat turns off the builtin, but how does your fix work then (see below)? For testing libm, I prevent use of builtins and even in functions in the standard library by renaming functions in a copy of the source code and testing the renamed functions. -fno-builtin should work for turning off builtins. > I have verified that both clang and GCC generate sensible code for the > builtin. For soft float they clear the sign bit using integer operations > and in hard float mode they use abs.d. Seems good enough for a single arch. Did you test O0 -fno-builtin? -O0 should be supported for debugging, but it is not so reasonable to compile libc with -fno-builtin. > Modified: head/lib/libc/mips/gen/fabs.c > ============================================================================== > --- head/lib/libc/mips/gen/fabs.c Tue Nov 28 19:57:16 2017 (r326341) > +++ head/lib/libc/mips/gen/fabs.c Tue Nov 28 20:37:27 2017 (r326342) > @@ -42,7 +42,6 @@ __FBSDID("$FreeBSD$"); > double > fabs(double x) > { > - if (x < 0) > - x = -x; > - return(x); > + > + return (__builtin_fabs(x)); > } Why doesn't the libc++ test suite on softfloat MIPS64 hide the bug by using the builtin instead of this function? If the builtin is turned off completely, then the above either doesn't compile or generates a call to the nonexistent public function __builtin_fabs(). The builtin isn't useable in general because some compilers don't have it, and ones that have it might have a fake version which calls the non-builtin version which gives endless recursion. This normally calls the function of the same name (fabs here) instead of a function with the underscored name. libm's semi-portable implementation for all arches is: X double X fabs(double x) X { X u_int32_t high; X GET_HIGH_WORD(high,x); X SET_HIGH_WORD(x,high&0x7fffffff); X return x; X } This works on any arch with IEEE754 doubles. It works by zeroing the sign bit. Compilers should turn this into the same code as the builtin, and clang on amd64 does (clang on i386 is not so good). However, older compilers tend to pessimize by copying through 32-bit integer registers or the stack just like the code asks for. Bruce