<div dir="ltr">On 23 August 2013 17:45, Matt Turner <<a href="mailto:mattst88@gmail.com" target="_blank">mattst88@gmail.com</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div class=""><div class="h5">On Fri, Aug 23, 2013 at 8:27 AM, Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> wrote: > On 22 August 2013 16:08, Matt Turner <<a href="mailto:mattst88@gmail.com">mattst88@gmail.com</a>> 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. </div></div>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? </blockquote></div> </div><div class="gmail_extra">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. </div><div class="gmail_extra">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. </div><div class="gmail_extra">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]. </div></div>