[Mesa-dev] [RFC PATCH] mesa: Replace _mesa_round_to_even() with roundeven().

[Carl Worth]

On Wed, Mar 11 2015, Matt Turner wrote:
> Eric's initial patch adding constant expression evaluation for
> ir_unop_round_even used nearbyint...

Hi Matt,

It's great to see this commit, (and with such a detailed message).
Rounding is one of those things that can be surprisingly difficult to
get correct, and there are a lot of moving pieces here, (software
specifications, hardware behavior, performance penalties of changing
modes, exceptions, etc.). So this is definitely an area worth spending
the effort to get things right.

I do have a feeling that there's something we should add to this commit.
My first difficulty in trying to review it was in determining what the
behavioral change in the patch actually is. Most of the commit message
is history (which is useful) but it was hard to find a succinct
description of "here's what's changed".

Reading between the lines of the history, I can guess the following
list of things are happening in this commit:

> Worse, IROUND() is implemented using the trunc(x + 0.5) trick which
> fails for x = nextafterf(0.5, 0.0).

1. Fix _mesa_round_to_even to return a correct value in this case.

	Since you've done the careful work to identify this boundary
	case, (which was subtle enough that it was missed by the
	original implementation), I think this should be verified with a
	unit test.

> Still worse, _mesa_round_to_even unexpectedly returns an int.

2. Fix _mesa_round_to_even to return a floating-point value

	This sounds logically independent from the above
	change. Usually, I'd say that justifies separate commits, but
	maybe combining these two in one is fine. What would at least be
	nice if the first paragraph of the commit message could list
	exactly what's changed, before all the motivation.

> rint() and nearbyint() implement the round-half-to-even behavior we want
> when the rounding mode is set to the default round-to-nearest. The only
> difference between them is that nearbyint() raises the inexact
> exception.
>
> This patch implements roundeven{f,}, a function added by a yet
> unimplemented technical specification (ISO/IEC TS 18661-1:2014), with a
> small difference in behavior -- we don't bother raising the inexact
> exception, which I don't think we care about anyway.

3. Rename _mesa_round_to_even to roundeven

	I'm less confident about this change. If we're using the name of
	a standardized function, (that may appear in glibc at any point
	in the future), shouldn't we at least have some configure
	machinery in place so that we can actually use the system
	version if available?

> If we do indeed want the don't-raise-the-inexact-exception behavior,
> maybe we should just keep the _mesa_round_to_even name?

If we care and don't want the exception, then yes, we'd better not use
the standard name. But even if we don't care and could live with the
exception, I don't think we should use a standardized name for a
function that we know differs in its behavior.

If we do decide to keep the rename, I propose at least splitting this
part into a separate commit.

> The SSE 4.1 ROUND instructions let us implement roundeven directly.
> Otherwise we assume that the rounding mode has not been modified (as we
> do in the rest of Mesa) and use rint().

4. Optimize this function (whatever its name) with SSE4.1 ROUND

	This looks like a reasonable optimization, but should be a
	separate commit, (and this commit would benefit from the unit
	tests mentioned before).

So I think I'd like to see at least three or four commits here:

	1. Change _mesa_round_to_even to return a float
	2. Fix rounding bug in _mesa_round_to_even
	3. Add unit test for recent rounding-bug fix
	4. Optimize _mesa_round_to_even with SSE 4.1 ROUND if available

With that: Reviewed-by: Carl Worth <cworth at cworth.org>

-Carl

-- 
carl.d.worth at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150311/7d079adb/attachment.sig>