[Mesa-dev] [PATCH 1/2] i965/fs: fix precision of f2b

[Jason Ekstrand]

On Thu, Feb 25, 2016 at 8:19 AM, Matt Turner <mattst88 at gmail.com> wrote:

> On Thu, Feb 25, 2016 at 2:15 AM, Iago Toral Quiroga <itoral at igalia.com>
> wrote:
> > From the OpenGL 4.2 spec:
> >
> > "When a constructor is used to convert any integer or floating-point
> type to a
> >  bool, 0 and 0.0 are converted to false, and non-zero values are
> converted to
> >  true."
> >
> > Thus, even the smallest non-zero floating value should be translated to
> true.
> > This behavior has been verified also with proprietary NVIDIA drivers.
>
> Ooh, interesting.
>
> > Currently, we implement this conversion as a cmp.nz operation with
> floats,
> > subject to floating-point precision limitations, and as a result,
> relatively
> > small non-zero floating point numbers return false instead of true.
> >
> > This patch fixes the problem by getting rid of the sign bit (to cover
> the case
> > of -0.0) and testing the result against 0u using an integer comparison
> instead.
> > ---
> >  src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> > index db20c71..7d62d7e 100644
> > --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> > @@ -913,9 +913,18 @@ fs_visitor::nir_emit_alu(const fs_builder &bld,
> nir_alu_instr *instr)
> >        bld.MOV(result, negate(op[0]));
> >        break;
> >
> > -   case nir_op_f2b:
> > -      bld.CMP(result, op[0], brw_imm_f(0.0f), BRW_CONDITIONAL_NZ);
> > -      break;
> > +   case nir_op_f2b: {
> > +      /* Because comparing to 0.0f is subject to precision limitations,
> do the
> > +       * comparison using integers (we need to get rid of the sign bit
> for that)
> > +       */
> > +      if (devinfo->gen >= 8)
> > +         op[0] = resolve_source_modifiers(op[0]);
>

Hrm... I'm not sure what I think about this.  Neither abs nor neg *should*
affect f2b since zero/non-zero should just pass through.  However,
resolving the source modifiers could affect if they, for instance, flushed
dnorms.  Incidentally, this means we probably want a NIR optimization to
get rid of neg and abs right before != 0 if we can.


>
> Oh, good thinking.
>
> > +      op[0] = retype(op[0], BRW_REGISTER_TYPE_UD);
> > +      bld.AND(op[0], op[0], brw_imm_ud(0x7FFFFFFFu));
> > +      bld.CMP(result, op[0], brw_imm_ud(0u), BRW_CONDITIONAL_NZ);
>
> The cmod_propagation is going to turn this into an AND.NZ, which we
> may as well just do here:
>
>    set_condmod(BRW_CONDITIONAL_NZ,
>                           bld.AND(result, op[0], brw_imm_ud(0x7FFFFFFFu));
>
> > +       break;
>
> Bad indentation.
>
> With those two things changed in both patches, they are
>
> Reviewed-by: Matt Turner <mattst88 at gmail.com>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160225/1d98b62d/attachment-0001.html>