[Mesa-dev] [PATCH] util/format: Generate floating point constants for clamping.
From: José Fonseca jfons...@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://bugs.freedesktop.org/show_bug.cgi?id=29661 Cc: 10.2 10.3 mesa-sta...@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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
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, jfons...@vmware.com wrote: From: José Fonseca jfons...@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://bugs.freedesktop.org/show_bug.cgi?id=29661 Cc: 10.2 10.3 mesa-sta...@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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] util/format: Generate floating point constants for clamping.
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.00 -2147483648
And to make things even stranger, it depends on compiler optimization:
$ gcc -O2 -Wall -o test test.c
$ ./test
2147483648.00 2147483647
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...
Jose
From: Chris Forbes chr...@ijw.co.nz
Sent: 07 November 2014 15:12
To: Jose Fonseca
Cc: mesa-dev@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, jfons...@vmware.com wrote:
From: José Fonseca jfons...@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-3D29661d=AAIFaQc=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEsr=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzEm=djQWlVnf6dbYVyPQ5qxDycdd3R5KbOq7Bj00kHD0dZEs=1xrbiqwOdtyjZXGw4e6Kj338lZaorHg_Hj-BNByAboQe=
Cc: 10.2 10.3 mesa-sta...@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@lists.freedesktop.org
https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddevd=AAIFaQc=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEsr=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzEm=djQWlVnf6dbYVyPQ5qxDycdd3R5KbOq7Bj00kHD0dZEs=imZ41uT_6T2fgHee2dCxYwjQLSqfLYKgC8YgzpGFbpMe=
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] util/format: Generate floating point constants for clamping.
Surprising indeed... Reviewed-by: Roland Scheidegger srol...@vmware.com Am 07.11.2014 um 15:32 schrieb jfons...@vmware.com: From: José Fonseca jfons...@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://bugs.freedesktop.org/show_bug.cgi?id=29661 Cc: 10.2 10.3 mesa-sta...@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) ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] util/format: Generate floating point constants for clamping.
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.00 -2147483648
And to make things even stranger, it depends on compiler optimization:
$ gcc -O2 -Wall -o test test.c
$ ./test
2147483648.00 2147483647
That's not surprising, since it's undefined. On x86 out-of-bounds values
return the integer indefinite value, which is 0x8000 (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 chr...@ijw.co.nz
Sent: 07 November 2014 15:12
To: Jose Fonseca
Cc: mesa-dev@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, jfons...@vmware.com wrote:
From: José Fonseca jfons...@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-3D29661d=AAIFaQc=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEsr=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzEm=djQWlVnf6dbYVyPQ5qxDycdd3R5KbOq7Bj00kHD0dZEs=1xrbiqwOdtyjZXGw4e6Kj338lZaorHg_Hj-BNByAboQe=
Cc: 10.2 10.3 mesa-sta...@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
