[Mesa-dev] [PATCH] glsl: ensure that frexp returns a 0 exponent for zero values

[Ilia Mirkin]

On Fri, Jul 18, 2014 at 5:27 PM, Matt Turner <mattst88 at gmail.com> wrote:
> On Fri, Jul 18, 2014 at 9:19 AM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>> The current code appears to work in simple tests, however this will
>> guarantee that the returned exponent is 0 for a 0 value.
>>
>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>> ---
>>
>> I couldn't make a simple test-case that would cause the current logic to
>> fail. However when I did the same thing with doubles, I ran into trouble. It
>> seems safer to move the csel outside of the add in case the value actually has
>> a non-0 exponent despite a 0 significand.
>>
>>  src/glsl/builtin_functions.cpp | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/glsl/builtin_functions.cpp b/src/glsl/builtin_functions.cpp
>> index e01742c..5755de9 100644
>> --- a/src/glsl/builtin_functions.cpp
>> +++ b/src/glsl/builtin_functions.cpp
>> @@ -4229,8 +4229,8 @@ builtin_builder::_frexp(const glsl_type *x_type, const glsl_type *exp_type)
>>      * to unsigned integers to ensure that 1 bits aren't shifted in.
>>      */
>>     body.emit(assign(exponent, rshift(bitcast_f2i(abs(x)), exponent_shift)));
>> -   body.emit(assign(exponent, add(exponent, csel(is_not_zero, exponent_bias,
>> -                                                     imm(0, vec_elem)))));
>> +   body.emit(assign(exponent, csel(is_not_zero, add(exponent, exponent_bias),
>> +                                   imm(0, vec_elem))));
>
> So you're changing the logic from
>
> exponent = (f2i(abs(x) >> 23) + (x != 0.0f) ? -126 : 0;
>
> to
>
> exponent = (x != 0.0f) ? (f2i(abs(x) >> 23) - 126 : 0;
>
> These seem identical to me, and trivially so for 0.0f/-0.0f. I have a
> feeling that this patch is papering over a bug in your code generation
> for f2i(abs(x)).

Could be, I'll take a closer look at the generated code. Thanks for the hint.

> In commit 9c48ae75 I fixed a bug in i965 where instead of generating
> f2i(abs(x)) we'd apply the source modifier effectively after the
> bitcast (reading the register with a different type), so we'd get
> abs(f2i(x)).
>
> For 0.0f, applying the f2i and abs out of order doesn't affect the
> result, but for -0.0f (0x80000000, -2147483648) instead of getting 0,
> you'd get abs(-2147483648) (which is likely -2147483648 itself!).

Couldn't you have a situation where 0.0 or -0.0 are actually not in
normal form, and are, e.g. 0xff800000 or something (i.e. non-0
exponent)? I was concerned about that situation. If the floats are
guaranteed to be in normalized form, then you're right that those two
should be identical.

  -ilia