<div dir="ltr"> On Dec 10, 2014 8:13 PM, "Michel Dänzer" <<a href="mailto:michel@daenzer.net" target="_blank">michel@daenzer.net</a>> wrote: > > On 11.12.2014 01:08, Jason Ekstrand wrote: > > > > > > On Tue, Dec 9, 2014 at 10:53 PM, Michel Dänzer <<a href="mailto:michel@daenzer.net" target="_blank">michel@daenzer.net</a> > > <mailto:<a href="mailto:michel@daenzer.net" target="_blank">michel@daenzer.net</a>>> wrote: > > > > On 09.12.2014 21:06, Iago Toral Quiroga wrote: > > > From: Jason Ekstrand <<a href="mailto:jason.ekstrand@intel.com" target="_blank">jason.ekstrand@intel.com</a> <mailto:<a href="mailto:jason.ekstrand@intel.com" target="_blank">jason.ekstrand@intel.com</a>>> > > > > > > This patch fixes the return of a wrong value when x is lower than > > > -MAX_INT(src_bits) as the result would not be between [-1.0 1.0]. > > > > > > v2 by Samuel Iglesias <<a href="mailto:siglesias@igalia.com" target="_blank">siglesias@igalia.com</a> <mailto:<a href="mailto:siglesias@igalia.com" target="_blank">siglesias@igalia.com</a>>>: > > > - Modify unorm_to_float() to avoid doing the division when > > > x == -MAX_INT(src_bits) > > > > > > Reviewed-by: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com" target="_blank">ian.d.romanick@intel.com</a> <mailto:<a href="mailto:ian.d.romanick@intel.com" target="_blank">ian.d.romanick@intel.com</a>>> > > > --- > > > src/mesa/main/format_utils.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/src/mesa/main/format_utils.c b/src/mesa/main/format_utils.c > > > index 93a0cea..5dd0848 100644 > > > --- a/src/mesa/main/format_utils.c > > > +++ b/src/mesa/main/format_utils.c > > > @@ -152,7 +152,7 @@ unorm_to_float(unsigned x, unsigned src_bits) > > > static inline float > > > snorm_to_float(int x, unsigned src_bits) > > > { > > > - if (x == -MAX_INT(src_bits)) > > > + if (x <= -MAX_INT(src_bits)) > > > return -1.0f; > > > else > > > return x * (1.0f / (float)MAX_INT(src_bits)); > > > > > > > The commit log talks about unorm_to_float, but the code modifies > > snorm_to_float. Also, I think something like > > > > mesa: Fix clamping to -1.0 in snorm_to_float > > > > would be a more useful shortlog. > > > > > > BTW, don't this function and unorm_to_float also need corresponding > > clamping to 1.0 for values >= MAX_INT(src_bits)? > > > > > > Yes and no. Every place that we use [us]norm_to_*, we assume that the > > value passed in is represented by the given number of bits. Therefore, > > clamping above should always be useless. The one edge case is the > > signed value MIN_INT(src_bits) (or, if you prefer, -MAX_INT(src_bits) - > > 1) in which is representable in the given number of bits but is actually > > less than -1 according to the formula. Technically, using an equality > > there was Ok, it's just that I had it equal to the wrong thing. > > So, is returning -1.0 for -MAX_INT(src_bits) correct then, or should the > test be '== MIN_INT(src_bits)' or '== -MAX_INT(src_bits) - 1' or either > variant using <=? -MAX_INT(src_bits) will map to -1.0 even without the if statement (just look at the formula). The problem is that, thanks to the clamping implicit in the conversion formulas, so should -MAX_INT(src_bits)-1. Therefore, any of "< -MAX_INT(src_bits)", "== -MAX_INT(src_bits) - 1", or "<= -MAX_INT(src_bits)" will work. The best (most clear) would probably be "< -MAX_INT(src_bits)". If I recall correctly, I decided on the other simply because, while we have the branch in there anyway, we might as well cut the calculation when we can. It doesn't really matter but maybe clearer is better here. In that case, "< -MAX_INT(src_bits)" would be bets.If we do add an upper bound (which I don't think is needed) it should definitely be "> MAX_INT(src_bits)" so that the compiler will optimize it away 90% of the time. (The value usually either comes in from an N-bit data type or we & with MAX_UINT(src_bits) before passing it in, so the compiler *should* be able to do that. No guarantees though.--Jason > > -- > Earthling Michel Dänzer | <a href="http://www.amd.com" target="_blank">http://www.amd.com</a> > Libre software enthusiast | Mesa and X developer </div>