On Tue, Feb 3, 2015 at 12:42 PM, Connor Abbott <cwabbo...@gmail.com> wrote:
> On Tue, Feb 3, 2015 at 3:27 PM, Kenneth Graunke <kenn...@whitecape.org> > wrote: > > On Tuesday, February 03, 2015 07:10:20 AM you wrote: > >> On Feb 3, 2015 2:35 AM, "Kenneth Graunke" <kenn...@whitecape.org> > wrote: > >> > Caught by lit_sat.shader_test. > >> > > >> > Signed-off-by: Kenneth Graunke <kenn...@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
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev