<div dir="ltr">On 21 January 2013 00:49, Chad Versace <span dir="ltr"><<a href="mailto:chad.versace@linux.intel.com" target="_blank">chad.versace@linux.intel.com</a>></span> wrote:<br><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">Signed-off-by: Chad Versace <<a href="mailto:chad.versace@linux.intel.com">chad.versace@linux.intel.com</a>><br>

---<br>
 src/mesa/drivers/dri/i965/brw_vec4.h           |   3 +<br>
 src/mesa/drivers/dri/i965/brw_vec4_emit.cpp    |   8 ++<br>
 src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 155 +++++++++++++++++++++++++<br>
 3 files changed, 166 insertions(+)<br>
<br>
diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h b/src/mesa/drivers/dri/i965/brw_vec4.h<br>
index e65b92c..43d0454 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_vec4.h<br>
+++ b/src/mesa/drivers/dri/i965/brw_vec4.h<br>
@@ -431,6 +431,9 @@ public:<br>
    void emit_math(enum opcode opcode, dst_reg dst, src_reg src0, src_reg src1);<br>
    src_reg fix_math_operand(src_reg src);<br>
<br>
+   void emit_pack_half_2x16(dst_reg dst, src_reg src0);<br>
+   void emit_unpack_half_2x16(dst_reg dst, src_reg src0);<br>
+<br>
    void swizzle_result(ir_texture *ir, src_reg orig_val, int sampler);<br>
<br>
    void emit_ndc_computation();<br>
diff --git a/src/mesa/drivers/dri/i965/brw_vec4_emit.cpp b/src/mesa/drivers/dri/i965/brw_vec4_emit.cpp<br>
index 747edc2..e395ada 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_vec4_emit.cpp<br>
+++ b/src/mesa/drivers/dri/i965/brw_vec4_emit.cpp<br>
@@ -808,6 +808,14 @@ vec4_generator::generate_code(exec_list *instructions)<br>
         brw_DP2(p, dst, src[0], src[1]);<br>
         break;<br>
<br>
+      case BRW_OPCODE_F32TO16:<br>
+         brw_F32TO16(p, dst, src[0]);<br>
+         break;<br>
+<br>
+      case BRW_OPCODE_F16TO32:<br>
+         brw_F16TO32(p, dst, src[0]);<br>
+         break;<br>
+<br>
       case BRW_OPCODE_IF:<br>
         if (inst->src[0].file != BAD_FILE) {<br>
            /* The instruction has an embedded compare (only allowed on gen6) */<br>
diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp<br>
index ebf8990..b5f1aae 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp<br>
+++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp<br>
@@ -348,6 +348,143 @@ vec4_visitor::emit_math(enum opcode opcode,<br>
 }<br>
<br>
 void<br>
+vec4_visitor::emit_pack_half_2x16(dst_reg dst, src_reg src0)<br>
+{<br>
+   if (intel->gen < 7)<br>
+      assert(!"ir_unop_pack_half_2x16 should be lowered");<br>
+<br>
+   /* uint dst; */<br>
+   assert(dst.type == BRW_REGISTER_TYPE_UD);<br>
+<br>
+   /* vec2 src0; */<br>
+   assert(src0.type == BRW_REGISTER_TYPE_F);<br>
+<br>
+   /* uvec2 tmp;<br>
+    *<br>
+    * The PRM lists the destination type of f32to16 as W.  However, I've<br>
+    * experimentally confirmed on gen7 that it must be a 32-bit size, such as<br>
+    * UD, in align16 mode.<br>
+    */<br>
+   dst_reg tmp_dst(this, glsl_type::uvec2_type);<br>
+   src_reg tmp_src(tmp_dst);<br>
+<br>
+   /* tmp.xy = f32to16(src0); */<br>
+   tmp_dst.writemask = WRITEMASK_XY;<br>
+   emit(new(mem_ctx) vec4_instruction(this, BRW_OPCODE_F32TO16,<br>
+                                      tmp_dst, src0));<br>
+<br>
+   /* The result's high 16 bits are in the low 16 bits of the temporary<br>
+    * register's Y channel.  The result's low 16 bits are in the low 16 bits<br>
+    * of the X channel.<br>
+    *<br>
+    * In experiments on gen7 I've found the that, in the temporary register,<br>
+    * the hight 16 bits of the X and Y channels are zeros. This is critical<br>
+    * for the SHL and OR instructions below to work as expected.<br>
+    */<br></blockquote><div><br></div><div>I checked the docs and I concur with Eric's reading--these higher-order bits don't seem to be guaranteed to be zero.  I'm guessing you got lucky in your tests.<br></div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+   /* dst = tmp.y << 16; */<br>
+   tmp_src.swizzle = SWIZZLE_Y;<br>
+   emit(new(mem_ctx) vec4_instruction(this, BRW_OPCODE_SHL,<br>
+                                      dst, tmp_src, src_reg(16u)));<br>
+   /* dst |= tmp.x; */<br>
+   tmp_src.swizzle = SWIZZLE_X;<br>
+   emit(new(mem_ctx) vec4_instruction(this, BRW_OPCODE_OR,<br>
+                                      dst, src_reg(dst), tmp_src));<br>
+<br>
+<br>
+   /* Idea for reducing the above number of registers and instructions<br>
+    * ----------------------------------------------------------------<br>
+    *<br>
+    * It should be possible to remove the temporary register and replace the<br>
+    * SHL and OR instructions above with a single MOV instruction mode in<br>
+    * align1 mode that uses clever register region addressing. (It is<br>
+    * impossible to specify the necessary register regions in align16 mode).<br>
+    * Unfortunately, it is difficult to emit an align1 instruction here.<br>
+    *<br>
+    * In particular, I want to do this:<br>
+    *<br>
+    *   # Give dst the form:<br>
+    *   #<br>
+    *   #    w z          y          x w z          y          x<br>
+    *   #  |0|0|0x0000hhhh|0x0000llll|0|0|0x0000hhhh|0x0000llll|<br>
+    *   #<br>
+    *   f32to16(8) dst<1>.xy:UD src<4;4,1>:F {align16}<br>
+    *<br>
+    *   # Transform dst into the form of packHalf2x16's output.<br>
+    *   #<br>
+    *   #    w z          y          x w z          y          x<br>
+    *   #  |0|0|0x00000000|0xhhhhllll|0|0|0x00000000|0xhhhhllll|<br>
+    *   #<br>
+    *   # Use width=2 in order to move the Y channel's high 16 bits<br>
+    *   # into the low 16 bits, thus clearing the Y channel to zero.<br>
+    *   #<br>
+    *   mov(4) dst.1<1>:UW dst.2<8;2,1>:UW {align1}<br>
+    */<br></blockquote><div><br></div><div>I don't think this mov instruction would do what you're hoping, for two reasons.<br><br>1. It gets the channel enables wrong.  In normal SIMD4x2 execution, the first 4 channel enables enable vertex 0, and the next 4 channel enables enable vertex 1.  Since you're using a mov(4) instruction, it's going to be entirely conditioned on the channel enables for vertex 0, even though it's handling data for both vertices.  Which means it'll do the wrong thing for non-uniform control flow.<br>
<br></div><div>2. You're assuming that the VertStride of 8 and Width of 2 will apply to the destination of the move.  They won't--destination width is always equal to ExecSize (meaning that the destination region is always a single horizontal row, so VertStride is irrelevant).  So the destination region you're actually specifying with this mov instruction is the upper two bytes of vertex0.x, all of vertex0.y, and the lower two bytes of vertex0.z.  That's not what you want.<br>
<br></div><div>I'm wracking my brain and I can't find a way to fix this mov instruction to do what we want :(<br><br></div><div>I don't want to leave the comment as is because it may mislead us into coming back later and writing incorrect code (especially since a channel enable bug is only going to manifest if there's non-uniform control flow, so we're likely to miss it in testing).  I'll leave it to your judgement whether it's better to note these problems in the comment or just delete it.  <br>
</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+}<br>
+<br>
+void<br>
+vec4_visitor::emit_unpack_half_2x16(dst_reg dst, src_reg src0)<br>
+{<br>
+   if (intel->gen < 7)<br>
+      assert(!"ir_unop_unpack_half_2x16 should be lowered");<br>
+<br>
+   /* vec2 dst; */<br>
+   assert(dst.type == BRW_REGISTER_TYPE_F);<br>
+<br>
+   /* uint src0; */<br>
+   assert(src0.type == BRW_REGISTER_TYPE_UD);<br>
+<br>
+   /* uvec2 tmp; */<br>
+   dst_reg tmp_dst(this, glsl_type::uvec2_type);<br>
+   src_reg tmp_src(tmp_dst);<br>
+<br>
+   /* tmp.x = src0 & 0xffffu; */<br>
+   tmp_dst.writemask = WRITEMASK_X;<br>
+   emit(new(mem_ctx) vec4_instruction(this, BRW_OPCODE_AND,<br>
+                                      tmp_dst, src0, src_reg(0xffffu)));<br>
+<br>
+   /* tmp.y = src0 >> 16u; */<br>
+   tmp_dst.writemask = WRITEMASK_Y;<br>
+   emit(new(mem_ctx) vec4_instruction(this, BRW_OPCODE_SHR,<br>
+                                      tmp_dst, src0, src_reg(16u)));<br>
+<br>
+   /* dst = f16to32(tmp); */<br>
+   dst.writemask = WRITEMASK_XY;<br>
+   emit(new(mem_ctx) vec4_instruction(this, BRW_OPCODE_F16TO32,<br>
+                                      dst, tmp_src));<br>
+<br>
+   /* Idea for reducing the above number of registers and instructions<br>
+    * ----------------------------------------------------------------<br>
+    *<br>
+    * It should be possible to remove the temporary register and replace the<br>
+    * SHR and AND instructions above with a single MOV instruction mode in<br>
+    * align1 mode that uses clever register region addressing. (It is<br>
+    * impossible to specify the necessary register regions in align16 mode).<br>
+    * Unfortunately, it is difficult to emit an align1 instruction here.<br>
+    *<br>
+    * In particular, I want to do this:<br>
+    *<br>
+    *   # Now, src has the form of unpackHalf2x16's input:<br>
+    *   #<br>
+    *   #    w z          y          x w z          y          x<br>
+    *   #  |0|0|0x00000000|0xhhhhllll|0|0|0x00000000|0xhhhhllll|<br>
+    *<br>
+    *   # Transform src into a form consumable by f16to32:<br>
+    *   #<br>
+    *   #    w z          y          x w z          y          x<br>
+    *   #  |0|0|0x0000hhhh|0x0000llll|0|0|0x0000hhhh|0x0000llll|<br>
+    *   #<br>
+    *   # Use dst as the scratch register.<br>
+    *   #<br>
+    *   mov(2) dst.2<1>:UW dst.1<8;1,1>:UW {align1}<br></blockquote><div><br></div><div>Once again, this gets the channel enables wrong.  And again I don't think that it's fixable :(</div><div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+    *<br>
+    *   # Give dst the form of unpackHalf2x16's output:<br>
+    *   #<br>
+    *   f16to32(4) dst<1>.xy:F src<4;4,1>:UD {align16}<br></blockquote><div><br></div><div>I think you mean f16to32(8) here--using an exec size of 4 would make the instruction apply only to vertex0 and not vertex1.  But I suppose it's moot if we can't figure out a way to fix the mov instruction above.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+    */<br>
+}<br>
+<br>
+void<br>
 vec4_visitor::visit_instructions(const exec_list *list)<br>
 {<br>
    foreach_list(node, list) {<br>
@@ -1469,6 +1606,24 @@ vec4_visitor::visit(ir_expression *ir)<br>
    case ir_quadop_vector:<br>
       assert(!"not reached: should be handled by lower_quadop_vector");<br>
       break;<br>
+<br>
+   case ir_unop_pack_half_2x16:<br>
+      emit_pack_half_2x16(result_dst, op[0]);<br>
+      break;<br>
+   case ir_unop_unpack_half_2x16:<br>
+      emit_unpack_half_2x16(result_dst, op[0]);<br>
+      break;<br>
+   case ir_unop_pack_snorm_2x16:<br>
+   case ir_unop_pack_unorm_2x16:<br>
+   case ir_unop_unpack_snorm_2x16:<br>
+   case ir_unop_unpack_unorm_2x16:<br>
+      assert(!"not reached: should be handled by lower_packing_builtins");<br>
+      break;<br>
+   case ir_unop_unpack_half_2x16_split_x:<br>
+   case ir_unop_unpack_half_2x16_split_y:<br>
+   case ir_binop_pack_half_2x16_split:<br>
+           assert(!"not reached: should not occur in vertex shader");<br>
+           break;<br>
    }<br>
 }<br>
<span class=""><font color="#888888"><br>
--<br>
1.8.1.1<br>
<br>
_______________________________________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
</font></span></blockquote></div><br></div><div class="gmail_extra"><br></div></div>