[Mesa-dev] [PATCH] i965/fs: Fix saturate for nir_opcode_bcsel.

[Jason Ekstrand]

On Tue, Feb 3, 2015 at 12:42 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:

> On Tue, Feb 3, 2015 at 3:27 PM, Kenneth Graunke <kenneth at whitecape.org>
> wrote:
> > On Tuesday, February 03, 2015 07:10:20 AM you wrote:
> >> On Feb 3, 2015 2:35 AM, "Kenneth Graunke" <kenneth at whitecape.org>
> wrote:
> >> > Caught by lit_sat.shader_test.
> >> >
> >> > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> >> > ---
> >> >  src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 1 +
> >> >  1 file changed, 1 insertion(+)
> >> >
> >> > diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> >> > b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> >> > index 153a1be..3c611af 100644
> >> > --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> >> > +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> >> > @@ -1084,6 +1084,7 @@ fs_visitor::nir_emit_alu(nir_alu_instr *instr)
> >> >        emit(CMP(reg_null_d, op[0], fs_reg(0), BRW_CONDITIONAL_NZ));
> >> >        inst = emit(SEL(result, op[1], op[2]));
> >> >        inst->predicate = BRW_PREDICATE_NORMAL;
> >> > +      inst->saturate = instr->dest.saturate;
> >> >        break;
> >> >
> >> >     default:
> >> > --
> >> > 2.2.2
> >>
> >> Hrm... I thought bcsel worked on integers.  You shouldn't be able to
> sat it
> >> anyway... This seems strange.
> >>
> >> As a side-note, this is one of the downsides to typeless that we should
> >> figure out how to solve.  Not 100% sure how at the moment.
> >
> > For LIT's Z component, I generate different code based on whether
> > drivers support native integers/prefer real booleans:
> >
> >    bcsel(fge(0.0f, src.x), 0.0f, pow(...))
> >
> > or
> >
> >    fcsel(sge(0.0f, src.x), 0.0f, pow(...))
> >
> > My thinking was that bcsel uses a real boolean condition, whereas fcsel
> > has to do condition != 0.0f...and that the type of the sources being
> > selected shouldn't really matter.  But I suppose it does if we're doing
> > saturate.
>
> Yes, fcsel is supposed to be for emulating csel for HW that doesn't
> natively support integers... that being said, if you think about it,
> testing if condition != 0.0f is basically the same as testing if
> condition != 0, so maybe it would be better if we just used both fcsel
> and bcsel interchangably (except the modifiers mean different things),
> similar to imov and fmov now, and when we lower to modifiers we can
> change one to the other if it allows us to inline a modifier. HW
> without integers wouldn't have NIR with ineg and iabs instructions,
> and we would still always emit fcsel in that case, so it would never
> see a bcsel anyways.
>

Not quite.  There's always -0.0 which is not integer 0.  *sigh*


>
> >
> > Incidentally, making an "fsat" ALU operation would solve that ambiguity,
> > wouldn't require special handling all over the place, could be optimized
> > in nir_opt_algebraic, and would probably be better for nouveau...
> >
> > Plus, I think we can probably just emit MOV.sat in the i965 backend, and
> > Matt's saturation propagation pass should clean it up for us.
>
> As Jason explained, we already have a fsat instruction but we lower it
> to . As an aside, are you running your code through the validator? It
> should assert fail on a bcsel with a saturate modifier, since integer
> destinations can't have the saturate modifier set, so we shouldn't be
> having this problem with GLSL IR.
>

The validator didn't check that.  I just sent a patch to make it check it
and also to not fold saturate into instructions that can't handle it.  That
should fix things up.

--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150203/36e14671/attachment.html>