<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Feb 3, 2015 at 12:42 PM, Connor Abbott <span dir="ltr"><<a href="mailto:cwabbott0@gmail.com" target="_blank">cwabbott0@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Tue, Feb 3, 2015 at 3:27 PM, Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> wrote:<br>
> On Tuesday, February 03, 2015 07:10:20 AM you wrote:<br>
>> On Feb 3, 2015 2:35 AM, "Kenneth Graunke" <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> wrote:<br>
>> > Caught by lit_sat.shader_test.<br>
>> ><br>
>> > Signed-off-by: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>><br>
>> > ---<br>
>> >  src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 1 +<br>
>> >  1 file changed, 1 insertion(+)<br>
>> ><br>
>> > diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp<br>
>> > b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp<br>
>> > index 153a1be..3c611af 100644<br>
>> > --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp<br>
>> > +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp<br>
>> > @@ -1084,6 +1084,7 @@ fs_visitor::nir_emit_alu(nir_alu_instr *instr)<br>
>> >        emit(CMP(reg_null_d, op[0], fs_reg(0), BRW_CONDITIONAL_NZ));<br>
>> >        inst = emit(SEL(result, op[1], op[2]));<br>
>> >        inst->predicate = BRW_PREDICATE_NORMAL;<br>
>> > +      inst->saturate = instr->dest.saturate;<br>
>> >        break;<br>
>> ><br>
>> >     default:<br>
>> > --<br>
>> > 2.2.2<br>
>><br>
>> Hrm... I thought bcsel worked on integers.  You shouldn't be able to sat it<br>
>> anyway... This seems strange.<br>
>><br>
>> As a side-note, this is one of the downsides to typeless that we should<br>
>> figure out how to solve.  Not 100% sure how at the moment.<br>
><br>
> For LIT's Z component, I generate different code based on whether<br>
> drivers support native integers/prefer real booleans:<br>
><br>
>    bcsel(fge(0.0f, src.x), 0.0f, pow(...))<br>
><br>
> or<br>
><br>
>    fcsel(sge(0.0f, src.x), 0.0f, pow(...))<br>
><br>
> My thinking was that bcsel uses a real boolean condition, whereas fcsel<br>
> has to do condition != 0.0f...and that the type of the sources being<br>
> selected shouldn't really matter.  But I suppose it does if we're doing<br>
> saturate.<br>
<br>
</div></div>Yes, fcsel is supposed to be for emulating csel for HW that doesn't<br>
natively support integers... that being said, if you think about it,<br>
testing if condition != 0.0f is basically the same as testing if<br>
condition != 0, so maybe it would be better if we just used both fcsel<br>
and bcsel interchangably (except the modifiers mean different things),<br>
similar to imov and fmov now, and when we lower to modifiers we can<br>
change one to the other if it allows us to inline a modifier. HW<br>
without integers wouldn't have NIR with ineg and iabs instructions,<br>
and we would still always emit fcsel in that case, so it would never<br>
see a bcsel anyways.<br></blockquote><div><br></div><div>Not quite.  There's always -0.0 which is not integer 0.  *sigh*<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
><br>
> Incidentally, making an "fsat" ALU operation would solve that ambiguity,<br>
> wouldn't require special handling all over the place, could be optimized<br>
> in nir_opt_algebraic, and would probably be better for nouveau...<br>
><br>
> Plus, I think we can probably just emit MOV.sat in the i965 backend, and<br>
> Matt's saturation propagation pass should clean it up for us.<br>
<br>
</span>As Jason explained, we already have a fsat instruction but we lower it<br>
to . As an aside, are you running your code through the validator? It<br>
should assert fail on a bcsel with a saturate modifier, since integer<br>
destinations can't have the saturate modifier set, so we shouldn't be<br>
having this problem with GLSL IR.<br>
</blockquote></div><br></div><div class="gmail_extra">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.<br><br></div><div class="gmail_extra">--Jason<br></div></div>