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 <cwo...@cworth.org>
-Carl
--
carl.d.wo...@intel.com
pgp9C0kDV4b3E.pgp
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
