[Mesa-dev] Fixup: Use C++ style constant member functions for is_one and is_zero.

[Paul Berry]

On 20 September 2011 01:05, Tolga Dalman <tolga.dalman at googlemail.com>wrote:

>
> I wouldn't exactly call it 'optimisation' :) However, I feel a bit uneasy
> about using the == operator on float variables
> as this could lead to unexpected errors.
>
> Best regards
> Tolga Dalman
>

Tolga, your uneasiness is certainly justified in terms of conventional
wisdom.  As someone who used to write numerical analysis software, I cringe
whenever I see floats compared using ==, because I know that this is almost
always a bad idea because of rounding errors.  But there are occasional
exceptions, and I think this is one of them, for two reasons:

(a) most of the time when one is tempted to compare two floats for equality,
it's worse to have a false negative (classifying two floats as unequal when
in fact they're only unequal because of rounding errors) than to have a
false positive (classifying two floats as equal when in fact they're not
quite equal because of rounding errors).  But since the is_one() and
is_zero() functions are used to identify opportunities for optimizing the
shader, a false negative just means that we might occasionally generate less
optimal code, and a false positive means the shader would produce incorrect
outputs.  In this case a false positive is worse, so we should err on the
side of classifying floats as unequal, even if they are very close.

(b) the vast majority of the 1.0 and 0.0 values that we need to identify
with the is_one() and is_zero() functions either come from literal 1.0 and
0.0 values (either in Mesa code or in the user's shaders) or from trivial
algebraic simplifications like 2.0 - 1.0 => 1.0.  Since these values can be
represented perfectly in floats, there isn't an opportunity for rounding
errors to accumulate.

I also found your proposed alternative a little surprising:

fabs(imm.f - 1.0) < std::numeric_limits<float>::epsilon()

When rounding errors accumulate, they typically wind up being much larger
than epsilon().  If your goal is to make the test tolerant of rounding
errors, wouldn't you need a value much larger than epsilon() as your
threshold?  Or is your goal to get the same computational effect without the
cringe-worthiness of using "=="?

In any case I think this discussion may be moot, because AFAICT this patch
doesn't apply on master--brw_vec4.cpp has never had a src_reg_is_zero() or
src_reg_is_one() function.  Or am I missing something?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110920/d9cb8bd0/attachment.html>