[Mesa-dev] [PATCH] util/format: Generate floating point constants for clamping.

[Roland Scheidegger]

Ah yes I forgot about that, even though I have fixed this same problem
elsewhere in the past...
It is indeed an annoying problem, luckily only affects 32bit numbers.

Am 07.11.2014 um 16:35 schrieb Jose Fonseca:
> Great point.
> 
> 
> The source values, src[], are already in floating point, so it's OK not to represent 2147483647 exactly, however, if the compiler 2147483647.0f upwards to 2147483648 then indeed the clamping won't work properly, as 2147483648 will overflow when converted to int32.
> 
> I've confirmed that gcc generates precisely the same assembly, so at least this change fix one.
> 
> But you're right, we do have a bug, before and after this change:
> 
> $ cat test.c 
> #include <stdint.h>
> #include <stdio.h>
> 
> #define CLAMP( X, MIN, MAX )  ( (X)<(MIN) ? (MIN) : ((X)>(MAX) ? (MAX) : (X)) )
> 
> int main()
> {
> 	float src = 2147483648.0f;
> 	int32_t dst = (int32_t)CLAMP(src, -2147483648, 2147483647);
> 	printf("%f %i\n", src, dst);
> 	return 0;
> }
> $ gcc -O0 -Wall -o test test.c
> $ ./test 
> 2147483648.000000 -2147483648
> 
> 
> And to make things even stranger, it depends on compiler optimization:
> 
> $ gcc -O2 -Wall -o test test.c
> $ ./test 
> 2147483648.000000 2147483647

That's not surprising, since it's undefined. On x86 out-of-bounds values
return the "integer indefinite" value, which is 0x80000000 (though might
be different if using sse or not I forgot).
With O2 it probably got optimized away completely by gcc and the logic
apparently got it "correct".

> 
> Bummer..
> 
> It's quite hard to come up with general rules to cover all this corner cases.  I suspect llvmpipe's LLVM IR generation suffers from similar issues .
> 
> To starters we need to add test cases for these...
I think we mostly get lucky because a lot of possible conversions are
things which aren't hit in practice.


> Jose
> 
> ________________________________________
> From: Chris Forbes <chrisf at ijw.co.nz>
> Sent: 07 November 2014 15:12
> To: Jose Fonseca
> Cc: mesa-dev at lists.freedesktop.org; Roland Scheidegger; Brian Paul
> Subject: Re: [Mesa-dev] [PATCH] util/format: Generate floating point constants for clamping.
> 
> A possible wrinkle here is that 2147483647 isn't exactly representable
> as a float (the adjacent floats are 2147483520 and 2147483648). Is the
> clamp actually doing the right thing at the top end?
> 
> On Sat, Nov 8, 2014 at 3:32 AM,  <jfonseca at vmware.com> wrote:
>> From: José Fonseca <jfonseca at vmware.com>
>>
>> This commit causes the generated C code to change as
>>
>>             union util_format_r32g32b32a32_sscaled pixel;
>>   -         pixel.chan.r = (int32_t)CLAMP(src[0], -2147483648, 2147483647);
>>   -         pixel.chan.g = (int32_t)CLAMP(src[1], -2147483648, 2147483647);
>>   -         pixel.chan.b = (int32_t)CLAMP(src[2], -2147483648, 2147483647);
>>   -         pixel.chan.a = (int32_t)CLAMP(src[3], -2147483648, 2147483647);
>>   +         pixel.chan.r = (int32_t)CLAMP(src[0], -2147483648.0f, 2147483647.0f);
>>   +         pixel.chan.g = (int32_t)CLAMP(src[1], -2147483648.0f, 2147483647.0f);
>>   +         pixel.chan.b = (int32_t)CLAMP(src[2], -2147483648.0f, 2147483647.0f);
>>   +         pixel.chan.a = (int32_t)CLAMP(src[3], -2147483648.0f, 2147483647.0f);
>>             memcpy(dst, &pixel, sizeof pixel);
>>
>> which surprisingly makes a difference for MSVC.
>>
>> Thanks to Juraj Svec for diagnosing this and drafting a fix.
>>
>> Fixes https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.freedesktop.org_show-5Fbug.cgi-3Fid-3D29661&d=AAIFaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzE&m=djQWlVnf6dbYVyPQ5qxDycdd3R5KbOq7Bj00kHD0dZE&s=1xrbiqwOdtyjZXGw4e6Kj338lZaorHg_Hj-BNByAboQ&e=
>>
>> Cc: 10.2 10.3 <mesa-stable at lists.freedesktop.org>
>> ---
>>  src/gallium/auxiliary/util/u_format_pack.py | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/gallium/auxiliary/util/u_format_pack.py b/src/gallium/auxiliary/util/u_format_pack.py
>> index 6ccf04c..90f348e 100644
>> --- a/src/gallium/auxiliary/util/u_format_pack.py
>> +++ b/src/gallium/auxiliary/util/u_format_pack.py
>> @@ -226,9 +226,9 @@ def native_to_constant(type, value):
>>      '''Get the value of unity for this type.'''
>>      if type.type == FLOAT:
>>          if type.size <= 32:
>> -            return "%ff" % value
>> +            return "%.1ff" % float(value)
>>          else:
>> -            return "%ff" % value
>> +            return "%.1f" % float(value)
>>      else:
>>          return str(int(value))
>>
>> @@ -251,8 +251,8 @@ def clamp_expr(src_channel, dst_channel, dst_native_type, value):
>>      dst_max = dst_channel.max()
>>
>>      # Translate the destination range to the src native value
>> -    dst_min_native = value_to_native(src_channel, dst_min)
>> -    dst_max_native = value_to_native(src_channel, dst_max)
>> +    dst_min_native = native_to_constant(src_channel, value_to_native(src_channel, dst_min))
>> +    dst_max_native = native_to_constant(src_channel, value_to_native(src_channel, dst_max))
>>
>>      if src_min < dst_min and src_max > dst_max:
>>          return 'CLAMP(%s, %s, %s)' % (value, dst_min_native, dst_max_native)
>> --
>> 1.9.1
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev&d=AAIFaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzE&m=djQWlVnf6dbYVyPQ5qxDycdd3R5KbOq7Bj00kHD0dZE&s=imZ41uT_6T2fgHee2dCxYwjQLSqfLYKgC8YgzpGFbpM&e=