Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sat, 22 Apr 2017 18:24:25 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] math: rewrite fma with mostly int arithmetics

A few thoughts, inline below. I'm not entirely opposed to this, if it
turns out to be better than the alternatives, but I would like to
understand whether it really is...

On Wed, Apr 19, 2017 at 12:41:40AM +0200, Szabolcs Nagy wrote:
> the freebsd fma code failed to raise underflow exception in some
> cases in nearest rounding mode (affects fmal too) e.g.
> 
>   fma(-0x1p-1000, 0x1.000001p-74, 0x1p-1022)
> 
> and the inexact exception may be raised spuriously since the fenv
> is not saved/restored around the exact multiplication algorithm
> (affects x86 fma too).

Is it difficult to determine when the multiplication part of an fma is
exact? If you can determine this quickly, you can just return x*y+z in
this special case and avoid all the costly operations. For normal
range, I think it's roughly just using ctz to count mantissa bits of x
and y, and checking whether the sum is <= 53. Some additional handling
for denormals is needed of course.

> another issue is that the underflow behaviour when the rounded result
> is the minimal normal number is target dependent, ieee754 allows two
> ways to raise underflow for inexact results: raise if the result before
> rounding is in the subnormal range (e.g. aarch64, arm, powerpc) or if
> the result after rounding with infinite exponent range is in the
> subnormal range (e.g. x86, mips, sh).
> 
> to avoid all these issues the algorithm was rewritten with mostly int
> arithmetics and float arithmetics is only used to get correct rounding
> and raise exceptions according to the behaviour of the target without
> any fenv.h dependency. it also unifies x86 and non-x86 fma.
> 
> fmaf is not affected, fmal need to be fixed too.
> 
> this algorithm depends on a_clz_64 and it required a nasty volatile
> hack: gcc seems to miscompile the FORCE_EVAL macro of libm.h on i386.

These are two particular aspects I don't like; (1) I'd rather reduce
the number of a_* primitives we have rather than add new ones, and (2)
I'd like to avoid volatile dances to get the compiler to do what it's
supposed to do. I know the latter might be inevitable in some cases,
though, because compilers are buggy... :(

> ---
>  src/math/fma.c | 582 ++++++++++++++++-----------------------------------------
>  1 file changed, 158 insertions(+), 424 deletions(-)
> 
> attaching the new fma.c instead of a diff, it's more readable.

Thanks, much preferred!

> depends on the a_clz_64 patch and previous scalbn fix.
> 
> fmal should be possible to do in a similar way.
> 
> i expect it to be faster than the previous code on most targets as
> the rounding mode is not changed and has less multiplications
> (it is faster on x86_64 and i386), the code size is a bit bigger
> though.

Kinda surprising on i386 -- I'd expect the 64x64 multiplications to be
costly compared to float ones.

> #include <stdint.h>
> #include <float.h>
> #include <math.h>
> #include "atomic.h"
> 
> static inline uint64_t asuint64(double x)
> {
> 	union {double f; uint64_t i;} u = {x};
> 	return u.i;
> }
> 
> static inline double asdouble(uint64_t x)
> {
> 	union {uint64_t i; double f;} u = {x};
> 	return u.f;
> }

These could just be written with compound literals, making macros an
option, though I don't know if there's any reason one would prefer
macros.

> struct num { uint64_t m; int e; int sign; };
> 
> static struct num normalize(uint64_t x)
> {
> 	int e = x>>52;
> 	int sign = e & 1<<11;
> 	e &= (1<<11)-1;
> 	x &= (1ull<<52)-1;
> 	if (!e) {
> 		int k = a_clz_64(x);
> 		x <<= k-11;
> 		e = -k+12;
> 	}
> 	x |= 1ull<<52;
> 	x <<= 1;
> 	e -= 0x3ff + 52 + 1;
> 	return (struct num){x,e,sign};
> }
> 
> static void mul(uint64_t *hi, uint64_t *lo, uint64_t x, uint64_t y)
> {
> 	uint64_t t1,t2,t3;
> 	uint64_t xlo = (uint32_t)x, xhi = x>>32;
> 	uint64_t ylo = (uint32_t)y, yhi = y>>32;
> 
> 	t1 = xlo*ylo;
> 	t2 = xlo*yhi + xhi*ylo;
> 	t3 = xhi*yhi;
> 	*lo = t1 + (t2<<32);
> 	*hi = t3 + (t2>>32) + (t1 > *lo);
> }
> 
> static int zeroinfnan(uint64_t x)
> {
> 	return 2*x-1 >= 2*asuint64(INFINITY)-1;
> }
> 
> double fma(double x, double y, double z)
> {
> 	#pragma STDC FENV_ACCESS ON
> 	uint64_t ix = asuint64(x);
> 	uint64_t iy = asuint64(y);
> 	uint64_t iz = asuint64(z);
> 
> 	if (zeroinfnan(ix) || zeroinfnan(iy))
> 		return x*y + z;
> 	if (zeroinfnan(iz)) {
> 		if (z == 0)
> 			return x*y + z;
> 		return z;
> 	}
> 
> 	/* normalize so top 10bits and last bit are 0 */
> 	struct num nx, ny, nz;
> 	nx = normalize(ix);
> 	ny = normalize(iy);
> 	nz = normalize(iz);

If the only constraint here is that top 10 bits and last bit are 0, I
don't see why clz is even needed. You can meet this constraint for
denormals by always multiplying by 2 and using a fixed exponent value.

> 	/* mul: r = x*y */
> 	uint64_t rhi, rlo, zhi, zlo;
> 	mul(&rhi, &rlo, nx.m, ny.m);
> 	/* either top 20 or 21 bits of rhi and last 2 bits of rlo are 0 */
> 
> 	/* align exponents */
> 	int e = nx.e + ny.e;
> 	int d = nz.e - e;
> 	/* shift bits z<<=kz, r>>=kr, so kz+kr == d, set e = e+kr (== ez-kz) */
> 	if (d > 0) {
> 		if (d < 64) {
> 			zlo = nz.m<<d;
> 			zhi = nz.m>>64-d;
> 		} else {
> 			zlo = 0;
> 			zhi = nz.m;
> 			e = nz.e - 64;
> 			d -= 64;
> 			if (d == 0) {
> 			} else if (d < 64) {
> 				rlo = rhi<<64-d | rlo>>d | !!(rlo<<64-d);
> 				rhi = rhi>>d;
> 			} else {
> 				rlo = 1;
> 				rhi = 0;
> 			}
> 		}
> 	} else {
> 		zhi = 0;
> 		d = -d;
> 		if (d == 0) {
> 			zlo = nz.m;
> 		} else if (d < 64) {
> 			zlo = nz.m>>d | !!(nz.m<<64-d);
> 		} else {
> 			zlo = 1;
> 		}
> 	}
> 
> 	/* add */
> 	int sign = nx.sign^ny.sign;
> 	int samesign = !(sign^nz.sign);
> 	int nonzero = 1;
> 	if (samesign) {
> 		/* r += z */
> 		rlo += zlo;
> 		rhi += zhi + (rlo < zlo);
> 	} else {
> 		/* r -= z */
> 		uint64_t t = rlo;
> 		rlo -= zlo;
> 		rhi = rhi - zhi - (t < rlo);
> 		if (rhi>>63) {
> 			rlo = -rlo;
> 			rhi = -rhi-!!rlo;
> 			sign = !sign;
> 		}
> 		nonzero = !!rhi;
> 	}
> 
> 	/* set rhi to top 63bit of the result (last bit is sticky) */
> 	if (nonzero) {
> 		e += 64;
> 		d = a_clz_64(rhi)-1;
> 		/* note: d > 0 */
> 		rhi = rhi<<d | rlo>>64-d | !!(rlo<<d);
> 	} else if (rlo) {
> 		d = a_clz_64(rlo)-1;
> 		if (d < 0)
> 			rhi = rlo>>1 | (rlo&1);
> 		else
> 			rhi = rlo<<d;
> 	} else {
> 		/* exact +-0 */
> 		return x*y + z;
> 	}
> 	e -= d;
> 
> 	/* convert to double */
> 	int64_t i = rhi; /* in [1<<62,(1<<63)-1] */
> 	if (sign)
> 		i = -i;
> 	double r = i; /* in [0x1p62,0x1p63] */
> 
> 	if (e < -1022-62) {
> 		/* result is subnormal before rounding */
> 		if (e == -1022-63) {
> 			double c = 0x1p63;
> 			if (sign)
> 				c = -c;
> 			if (r == c) {
> 				/* min normal after rounding, underflow depends
> 				   on arch behaviour which can be imitated by
> 				   a double to float conversion */
> 				float fltmin = 0x0.ffffff8p-63*FLT_MIN * r;
> 				return DBL_MIN/FLT_MIN * fltmin;
> 			}
> 			/* one bit is lost when scaled, add another top bit to
> 			   only round once at conversion if it is inexact */
> 			if (rhi << 53) {
> 				i = rhi>>1 | (rhi&1) | 1ull<<62;
> 				if (sign)
> 					i = -i;
> 				r = i;
> 				r = 2*r - c; /* remove top bit */
> 				volatile double uflow = DBL_MIN/FLT_MIN;
> 				uflow *= uflow;
> 			}
> 		} else {
> 			/* only round once when scaled */
> 			d = 10;
> 			i = ( rhi>>d | !!(rhi<<64-d) ) << d;
> 			if (sign)
> 				i = -i;
> 			r = i;
> 		}
> 	}
> 	return scalbn(r, e);
> }

Powered by blists - more mailing lists

Your e-mail address:

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.