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

[Matt Turner]

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?