[Mesa-dev] [PATCH v3 01/24] mesa/format_utils: Fix a bug in unorm_to_float helper function

[Jason Ekstrand]

On Dec 10, 2014 8:13 PM, "Michel Dänzer" <michel at daenzer.net> wrote:
>
> On 11.12.2014 01:08, Jason Ekstrand wrote:
> >
> >
> > On Tue, Dec 9, 2014 at 10:53 PM, Michel Dänzer <michel at daenzer.net
> > <mailto:michel at daenzer.net>> wrote:
> >
> >     On 09.12.2014 21:06, Iago Toral Quiroga wrote:
> >     > From: Jason Ekstrand <jason.ekstrand at intel.com <mailto:
jason.ekstrand at intel.com>>
> >     >
> >     > 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 <siglesias at igalia.com <mailto:
siglesias at igalia.com>>:
> >     >     - Modify unorm_to_float() to avoid doing the division when
> >     >       x == -MAX_INT(src_bits)
> >     >
> >     > Reviewed-by: Ian Romanick <ian.d.romanick at intel.com <mailto:
ian.d.romanick at intel.com>>
> >     > ---
> >     >  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               |               http://www.amd.com
> Libre software enthusiast             |             Mesa and X developer
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141210/f28f853a/attachment.html>