[Mesa-dev] [PATCH 03/15] i965/fs: Add support for translating ir_triop_fma into MAD.

[Paul Berry]

On 23 August 2013 17:45, Matt Turner <mattst88 at gmail.com> wrote:

> On Fri, Aug 23, 2013 at 8:27 AM, Paul Berry <stereotype441 at gmail.com>
> wrote:
> > On 22 August 2013 16:08, Matt Turner <mattst88 at 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].
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130826/b6c3730b/attachment-0001.html>