On 23 August 2013 17:45, Matt Turner <matts...@gmail.com> wrote:
> On Fri, Aug 23, 2013 at 8:27 AM, Paul Berry <stereotype...@gmail.com>
> wrote:
> > On 22 August 2013 16:08, Matt Turner <matts...@gmail.com> wrote:
> >>
> >> ---
> >> src/mesa/drivers/dri/i965/brw_fs.cpp | 1 +
> >> src/mesa/drivers/dri/i965/brw_fs.h | 1 +
> >> src/mesa/drivers/dri/i965/brw_fs_channel_expressions.cpp | 1 +
> >> src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 7 +++++++
> >> 4 files changed, 10 insertions(+)
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> b/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> index 52fa6f4..b770c0e 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> @@ -179,6 +179,7 @@ ALU3(BFI2)
> >> ALU1(FBH)
> >> ALU1(FBL)
> >> ALU1(CBIT)
> >> +ALU3(MAD)
> >>
> >> /** Gen4 predicated IF. */
> >> fs_inst *
> >> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h
> >> b/src/mesa/drivers/dri/i965/brw_fs.h
> >> index 9d240b5..cb4ac3b 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_fs.h
> >> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
> >> @@ -285,6 +285,7 @@ public:
> >> fs_inst *FBH(fs_reg dst, fs_reg value);
> >> fs_inst *FBL(fs_reg dst, fs_reg value);
> >> fs_inst *CBIT(fs_reg dst, fs_reg value);
> >> + fs_inst *MAD(fs_reg dst, fs_reg c, fs_reg b, fs_reg a);
> >>
> >> int type_size(const struct glsl_type *type);
> >> fs_inst *get_instruction_generating_reg(fs_inst *start,
> >> diff --git a/src/mesa/drivers/dri/i965/brw_fs_channel_expressions.cpp
> >> b/src/mesa/drivers/dri/i965/brw_fs_channel_expressions.cpp
> >> index 4afae24..fa02d9b 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_fs_channel_expressions.cpp
> >> +++ b/src/mesa/drivers/dri/i965/brw_fs_channel_expressions.cpp
> >> @@ -360,6 +360,7 @@
> >> ir_channel_expressions_visitor::visit_leave(ir_assignment *ir)
> >> assert(!"not yet supported");
> >> break;
> >>
> >> + case ir_triop_fma:
> >> case ir_triop_lrp:
> >> case ir_triop_bitfield_extract:
> >> for (i = 0; i < vector_elements; i++) {
> >> diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> >> b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> >> index 964ad40..ac85d25 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> >> +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> >> @@ -717,6 +717,13 @@ fs_visitor::visit(ir_expression *ir)
> >> break;
> >> }
> >>
> >> + case ir_triop_fma:
> >> + /* Note that the instruction's argument order is reversed from
> GLSL
> >> + * and the IR.
> >> + */
> >> + emit(MAD(this->result, op[2], op[1], op[0]));
> >> + break;
> >> +
> >
> >
> > What happens if one of the ops is in a form that we can't encode in a
> 3-op
> > instruction (e.g. a constant)? That's handled in patch 4/15 for the vs,
> and
> > it's handled inside emit_lrp, but I don't see it handled here.
>
> I read your reply and thought "oh crap, I bet that doesn't work" but
> it actually does work. I honestly don't have any idea how or why.
>
> With fma(a, b, c) I get:
>
> mad(8) g14<1>F g3<4,1,1>F.x g2.4<4,1,1>F.x g2<4,1,1>F.x
> mad(8) g13<1>F g3.1<4,1,1>F.x g2.5<4,1,1>F.x g2.1<4,1,1>F.x
> mad(8) g12<1>F g3.2<4,1,1>F.x g2.6<4,1,1>F.x g2.2<4,1,1>F.x
> mad(8) g11<1>F g3.3<4,1,1>F.x g2.7<4,1,1>F.x g2.3<4,1,1>F.x
>
> With fma(a, vec4(1.0, 1.0, 2.0, 2.0), c) I get:
>
> mov(8) g13<1>F 1F
> mov(8) g10<1>F 1F
> mov(8) g7<1>F 2F
> mov(8) g4<1>F 2F
> mad(8) g14<1>F g2.4<4,1,1>F.x g13<4,1,1>F g2<4,1,1>F.x
> mad(8) g11<1>F g2.5<4,1,1>F.x g10<4,1,1>F g2.1<4,1,1>F.x
> mad(8) g8<1>F g2.6<4,1,1>F.x g7<4,1,1>F g2.2<4,1,1>F.x
> mad(8) g5<1>F g2.7<4,1,1>F.x g4<4,1,1>F g2.3<4,1,1>F.x
>
> The IR just looks like it contains inline (constant float (...)) in
> the fma expression, so it doesn't seem to be something in the frontend
> doing it.
>
> Any guess what's going on?
>
Aha! I see what it is. If you look at fs_visitor::visit(ir_constant *),
you'll see that it emits MOVs. Which means that in the code we were
discussing, op[i] will never be a constant.
I double checked this with Eric, and both of us think that op[i] will only
ever be of type GRF or UNIFORM, so probably this code is fine.
But it would be nice to have an assertion near the top of
fs_visitor::visit(ir_expression *) to verify that this->result is 3-source
compatible before we store it in op[operand].
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
