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

2013-08-27 Thread Paul Berry 
On 26 August 2013 22:00, Matt Turner matts...@gmail.com wrote:

 Thanks Paul.

 I've placed this patch as 2.5/15 in the series.

 i965/fs: Assert that ir_expressions are usable by 3-src instructions

 MAD will be generated directly from ir_triop_fma, so this assertion
 checks that all ir_expressions are usable.


Reviewed-by: Paul Berry stereotype...@gmail.com


 ---
 diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
 b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
 index 27887d6..a02d92d 100644
 --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
 +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
 @@ -336,6 +336,7 @@ fs_visitor::visit(ir_expression *ir)
  ir-operands[operand]-print();
   printf(\n);
}
 +  assert(this-result.is_valid_3src());


I spent a while trying to think of a commet that could go above the assert,
to help someone who stumbles across it in the future and wonders why the
assertion is here rather than in the switch statement below, but I couldn't
really think of anything :(


op[operand] = this-result;

/* Matrix expression operands should have been broken down to vector

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2013-08-26 Thread Paul Berry 
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) g141F g34,1,1F.x   g2.44,1,1F.x g24,1,1F.x
 mad(8) g131F g3.14,1,1F.x g2.54,1,1F.x g2.14,1,1F.x
 mad(8) g121F g3.24,1,1F.x g2.64,1,1F.x g2.24,1,1F.x
 mad(8) g111F g3.34,1,1F.x g2.74,1,1F.x g2.34,1,1F.x

 With fma(a, vec4(1.0, 1.0, 2.0, 2.0), c) I get:

 mov(8) g131F 1F
 mov(8) g101F 1F
 mov(8) g71F  2F
 mov(8) g41F  2F
 mad(8) g141F g2.44,1,1F.x g134,1,1F g24,1,1F.x
 mad(8) g111F g2.54,1,1F.x g104,1,1F g2.14,1,1F.x
 mad(8) g81F  g2.64,1,1F.x g74,1,1F  g2.24,1,1F.x
 mad(8) g51F  g2.74,1,1F.x g44,1,1F  g2.34,1,1F.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

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

2013-08-26 Thread Matt Turner 
Thanks Paul.

I've placed this patch as 2.5/15 in the series.

i965/fs: Assert that ir_expressions are usable by 3-src instructions

MAD will be generated directly from ir_triop_fma, so this assertion
checks that all ir_expressions are usable.
---
diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
index 27887d6..a02d92d 100644
--- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
+++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
@@ -336,6 +336,7 @@ fs_visitor::visit(ir_expression *ir)
 ir-operands[operand]-print();
  printf(\n);
   }
+  assert(this-result.is_valid_3src());
   op[operand] = this-result;

   /* Matrix expression operands should have been broken down to vector
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2013-08-23 Thread Paul Berry 
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.


 case ir_triop_lrp:
emit_lrp(this-result, op[0], op[1], op[2]);
break;
 --
 1.8.3.2

 ___
 mesa-dev mailing list
 mesa-dev@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/mesa-dev

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2013-08-23 Thread Matt Turner 
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) g141F g34,1,1F.x   g2.44,1,1F.x g24,1,1F.x
mad(8) g131F g3.14,1,1F.x g2.54,1,1F.x g2.14,1,1F.x
mad(8) g121F g3.24,1,1F.x g2.64,1,1F.x g2.24,1,1F.x
mad(8) g111F g3.34,1,1F.x g2.74,1,1F.x g2.34,1,1F.x

With fma(a, vec4(1.0, 1.0, 2.0, 2.0), c) I get:

mov(8) g131F 1F
mov(8) g101F 1F
mov(8) g71F  2F
mov(8) g41F  2F
mad(8) g141F g2.44,1,1F.x g134,1,1F g24,1,1F.x
mad(8) g111F g2.54,1,1F.x g104,1,1F g2.14,1,1F.x
mad(8) g81F  g2.64,1,1F.x g74,1,1F  g2.24,1,1F.x
mad(8) g51F  g2.74,1,1F.x g44,1,1F  g2.34,1,1F.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?
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2013-08-22 Thread Matt Turner 
---
 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;
+
case ir_triop_lrp:
   emit_lrp(this-result, op[0], op[1], op[2]);
   break;
-- 
1.8.3.2

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev