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

[Connor Abbott]

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.

>
> 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.

>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>