<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On 2 December 2013 11:39, Francisco Jerez <span dir="ltr"><<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">diff --git a/src/mesa/drivers/dri/i965/brw_vec4_surface_visitor.cpp b/src/mesa/drivers/dri/i965/brw_vec4_surface_visitor.cpp<br>
new file mode 100644<br>
index 0000000..3528bbe<br>
--- /dev/null<br>
+++ b/src/mesa/drivers/dri/i965/brw_vec4_surface_visitor.cpp<br>
@@ -0,0 +1,846 @@<br>
+/*<br>
+ * Copyright 2013 Intel Corporation<br>
+ *<br>
+ * Permission is hereby granted, free of charge, to any person obtaining a<br>
+ * copy of this software and associated documentation files (the "Software"),<br>
+ * to deal in the Software without restriction, including without limitation<br>
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,<br>
+ * and/or sell copies of the Software, and to permit persons to whom the<br>
+ * Software is furnished to do so, subject to the following conditions:<br>
+ *<br>
+ * The above copyright notice and this permission notice (including the next<br>
+ * paragraph) shall be included in all copies or substantial portions of the<br>
+ * Software.<br>
+ *<br>
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR<br>
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,<br>
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL<br>
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER<br>
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING<br>
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS<br>
+ * IN THE SOFTWARE.<br>
+ *<br>
+ * Authors:<br>
+ * Francisco Jerez <<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>><br>
+ */<br>
+<br>
+#include "brw_vec4_surface_visitor.h"<br>
+<br>
+using namespace brw;<br>
+<br>
+namespace {<br>
+ vec4_instruction &<br>
+ exec_all(vec4_instruction &inst)<br>
+ {<br>
+ inst.force_writemask_all = true;<br>
+ return inst;<br>
+ }<br>
+<br>
+ vec4_instruction &<br>
+ exec_predicated(backend_reg flag, vec4_instruction &inst)<br>
+ {<br>
+ if (flag.file != BAD_FILE)<br>
+ inst.predicate = BRW_PREDICATE_ALIGN16_ALL4H;<br></blockquote><div><br></div><div>There should be a comment here explaining why you chose this predicate (or at the very least referring the reader to emit_coordinate_check() for context).<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>
+ return inst;<br>
+ }<br>
+}<br>
+<br>
+<br>
+/**<br>
+ * Copy a SIMD4x2 vector to its transpose SIMD8x4 vector.<br></blockquote><div><br></div><div>Usually SIMDNxM means "M separate execution flows are simultaneously processed, with each execution flow processing a packed vector of N elements at a time". (Note that SIMD8 is considered an abbreviation for SIMD1x8 in this interpretation). So SIMD8x4 doesn't make any sense.<br>
<br>How about instead saying "Copy a SIMD4x2 vector to execution channels 0 and 4 of a SIMD8 vector"?<br><br></div><div>Also, there should be a comment clarifying why it's ok to leave channels 1-3 and 5-7 uninitialized.<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>
+void<br>
+brw_vec4_surface_visitor::emit_assign_to_transpose(<br>
+ dst_reg dst, src_reg src, unsigned size) const<br>
+{<br>
+ for (unsigned i = 0; i < size; ++i) {<br>
+ emit(BRW_OPCODE_MOV,<br>
+ writemask(offset(dst, i), WRITEMASK_X),<br>
+ swizzle(src, BRW_SWIZZLE4(i, i, i, i)));<br>
+ } <br></blockquote><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>
+ * Copy a SIMD4x2 vector from its transpose SIMD8x4 vector.<br></blockquote><div><br></div><div>Similar clarification is needed here.<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>
+void<br>
+brw_vec4_surface_visitor::emit_assign_from_transpose(<br>
+ dst_reg dst, src_reg src, unsigned size) const<br>
+{<br>
+ for (unsigned i = 0; i < size; ++i) {<br>
+ emit(BRW_OPCODE_MOV,<br>
+ writemask(dst, 1 << i),<br>
+ swizzle(offset(src, i), BRW_SWIZZLE_XXXX));<br>
+ }<br>
+}<br>
+<br>
+/**<br>
+ * Initialize the header present in some surface access messages.<br>
+ */<br>
+void<br>
+brw_vec4_surface_visitor::emit_surface_header(struct dst_reg dst) const<br>
+{<br>
+ assert(dst.file == MRF);<br>
+ exec_all(emit(BRW_OPCODE_MOV, dst, 0));<br>
+<br>
+ if (!v->brw->is_haswell) {<br>
+ /* The sample mask is used on IVB for the SIMD8 messages that<br>
+ * have no SIMD4x2 counterpart. We only use the two X channels<br>
+ * in that case, mask everything else out.<br>
+ */<br>
+ exec_all(emit(BRW_OPCODE_MOV,<br>
+ brw_writemask(brw_uvec_mrf(4, dst.reg, 4), WRITEMASK_W),<br>
+ 0x11));<br>
+ }<br>
+}<br></blockquote><div><br>As with the corresponding fs function, it would be
helpful for the comment to say which messages this function is expected
to be used for, or to include a bspec reference that shows the header format for the messages you care about.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+backend_reg<br>
+brw_vec4_surface_visitor::emit_coordinate_address_calculation(<br>
+ backend_reg image, backend_reg addr, unsigned dims) const<br></blockquote><div><br></div><div>Most of my comments on brw_fs_surface_visitor::emit_coordinate_address_calculation() apply here as well.<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>
+ const unsigned mask = (1 << dims) - 1;<br>
+ src_reg off = offset(image, BRW_IMAGE_PARAM_OFFSET_OFFSET / 4);<br>
+ src_reg stride = offset(image, BRW_IMAGE_PARAM_STRIDE_OFFSET / 4);<br>
+ src_reg tile = offset(image, BRW_IMAGE_PARAM_TILING_OFFSET / 4);<br>
+ src_reg swz = offset(image, BRW_IMAGE_PARAM_SWIZZLING_OFFSET / 4);<br>
+ src_reg dst = make_grf(BRW_REGISTER_TYPE_UD, 1);<br>
+ src_reg tmp = make_grf(BRW_REGISTER_TYPE_UD, 4);<br>
+<br>
+ /* Shift the coordinates by the fixed surface offset. */<br>
+ emit(BRW_OPCODE_ADD, writemask(addr, WRITEMASK_XY & mask),<br>
+ addr, off);<br>
+<br>
+ if (dims > 2) {<br>
+ /* Decompose z into a major (tmp.w) and a minor (tmp.z)<br>
+ * index.<br>
+ */<br>
+ emit(BRW_OPCODE_SHL, writemask(tmp, WRITEMASK_Z),<br>
+ addr, negate(tile));<br>
+<br>
+ emit(BRW_OPCODE_SHR, writemask(tmp, WRITEMASK_Z),<br>
+ tmp, negate(tile));<br></blockquote><div><br></div><div>The use of negate() with a shift is sufficiently clever that it could use a comment explaining what's going on. In partuclar, that the SHL and SHR are effectively preserving the lowest-order tile.z bits of addr.z and discarding the rest, except in the case where tile.z == 0, in which case they are keeping all the bits.<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>
+ emit(BRW_OPCODE_SHR, writemask(tmp, WRITEMASK_W),<br>
+ swizzle(addr, BRW_SWIZZLE_ZZZZ),<br>
+ swizzle(tile, BRW_SWIZZLE_ZZZZ));<br>
+<br>
+ /* Calculate the horizontal (tmp.z) and vertical (tmp.w) slice<br>
+ * offset.<br>
+ */<br>
+ emit(BRW_OPCODE_MUL, writemask(tmp, WRITEMASK_ZW),<br>
+ stride, tmp);<br>
+ emit(BRW_OPCODE_ADD, writemask(addr, WRITEMASK_XY),<br>
+ addr, swizzle(tmp, BRW_SWIZZLE_ZWZW));<br></blockquote><div><br></div><div>There should be a comment clarifying that in the case where tile.z == 0, since tmp.z == addr.z, we are relying on the fact that stride.z is 0 in order to avoid corrupting the contents of addr.x. Additionally, in patch 04/25, a comment should be added to brw_miptree_get_horizontal_slice_pitch() to clarify that in the non-GL_TEXTURE_3D case, brw_vec4_surface_visitor::emit_coordinate_address_calculation() relies on the return value being 0.<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>
+ if (dims > 1) {<br>
+ /* Calculate the minor x (tmp.x) and y (tmp.y) indices. */<br>
+ emit(BRW_OPCODE_SHL, writemask(tmp, WRITEMASK_XY),<br>
+ addr, negate(tile));<br>
+<br>
+ emit(BRW_OPCODE_SHR, writemask(tmp, WRITEMASK_XY),<br>
+ tmp, negate(tile));<br>
+<br>
+ /* Calculate the major x (tmp.z) and y (tmp.w) indices. */<br>
+ emit(BRW_OPCODE_SHR, writemask(tmp, WRITEMASK_ZW),<br>
+ swizzle(addr, BRW_SWIZZLE_XYXY),<br>
+ swizzle(tile, BRW_SWIZZLE_XYXY));<br>
+<br>
+ /* Multiply the minor indices and the major x index (tmp.x,<br>
+ * tmp.y and tmp.w) by the Bpp, and the major y index (tmp.w) by<br>
+ * the vertical stride.<br>
+ */<br>
+ emit(BRW_OPCODE_MUL, writemask(tmp, WRITEMASK_XYZW),<br>
+ swizzle(stride, BRW_SWIZZLE_XXXY), tmp);<br>
+<br>
+ /* Multiply by the tile dimensions using two shift instructions.<br>
+ * Equivalent to:<br>
+ * minor.y = minor.y << tile.x<br>
+ * major.x = major.x << tile.x << tile.y<br>
+ * major.y = major.y << tile.y<br>
+ */<br>
+ emit(BRW_OPCODE_SHL, writemask(tmp, WRITEMASK_ZW),<br>
+ swizzle(tmp, BRW_SWIZZLE_ZWZW),<br>
+ swizzle(tile, BRW_SWIZZLE_YYYY));<br>
+<br>
+ emit(BRW_OPCODE_SHL, writemask(tmp, WRITEMASK_YZ),<br>
+ swizzle(tmp, BRW_SWIZZLE_YYZZ),<br>
+ swizzle(tile, BRW_SWIZZLE_XXXX));<br>
+<br>
+ /* Add everything up. */<br>
+ emit(BRW_OPCODE_ADD, writemask(tmp, WRITEMASK_XY),<br>
+ swizzle(tmp, BRW_SWIZZLE_XYXY),<br>
+ swizzle(tmp, BRW_SWIZZLE_ZWZW));<br>
+<br>
+ emit(BRW_OPCODE_ADD, writemask(dst, WRITEMASK_X),<br>
+ swizzle(tmp, BRW_SWIZZLE_XXXX),<br>
+ swizzle(tmp, BRW_SWIZZLE_YYYY));<br></blockquote><div><br></div><div>I believe this code calculates the wrong value when tile.x == tile.y == 0, because all the shifts shift by zero, so just before the line "/* Add everything up. */", tmp has the values:<br>
<br>tmp.x = addr.x * stride.x<br></div><div>tmp.y = addr.y * stride.x<br></div><div>tmp.z = addr.x * stride.x<br></div><div>tmp.w = addr.y * stride.y<br><br></div><div>And then when you add it all up you get<br><br></div>
<div>dst.x = addr.x * (2 * stride.x) + addr.y * (stride.x + stride.y)<br><br></div><div>Which isn't correct. What you want is:<br><br></div><div>dst.x = addr.x * stride.x + addr.y * stride.y<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>
+ if (v->brw->has_swizzling) {<br>
+ /* Take into account the two dynamically specified shifts. */<br>
+ emit(BRW_OPCODE_SHR, writemask(tmp, WRITEMASK_XY),<br>
+ swizzle(dst, BRW_SWIZZLE_XXXX), swz);<br>
+<br>
+ /* XOR tmp.x and tmp.y with bit 6 of the memory address. */<br>
+ emit(BRW_OPCODE_XOR, writemask(tmp, WRITEMASK_X),<br>
+ swizzle(tmp, BRW_SWIZZLE_XXXX),<br>
+ swizzle(tmp, BRW_SWIZZLE_YYYY));<br>
+<br>
+ emit(BRW_OPCODE_AND, writemask(tmp, WRITEMASK_X),<br>
+ tmp, 1 << 6);<br>
+<br>
+ emit(BRW_OPCODE_XOR, writemask(dst, WRITEMASK_X),<br>
+ dst, tmp);<br>
+ }<br>
+<br>
+ } else {<br>
+ /* Multiply by the Bpp value. */<br>
+ emit(BRW_OPCODE_MUL, writemask(dst, WRITEMASK_X),<br>
+ addr, stride);<br>
+ }<br>
+<br>
+ return dst;<br>
+}<br>
+<br>
+backend_reg<br>
+brw_vec4_surface_visitor::emit_untyped_read(<br>
+ backend_reg flag, backend_reg surface, backend_reg addr,<br>
+ unsigned dims, unsigned size) const<br></blockquote><div><br></div><div>It would be nice to have a comment in this function saying that for both IVB and HSW, we use SIMD4x2 messages for untyped surface read.<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>
+void<br>
+brw_vec4_surface_visitor::emit_untyped_write(<br>
+ backend_reg flag, backend_reg surface, backend_reg addr,<br>
+ backend_reg src, unsigned dims, unsigned size) const<br>
+{<br>
+ const unsigned mask = (v->brw->is_haswell ? (1 << size) - 1 : 1);<br>
+ unsigned mlen = 0;<br></blockquote><div><br></div><div>It would be nice to move the comment about IVB using a SIMD8 message up here, since without that context, the code to emit the surface write address and the source value don't make sense. A similar comment applies to emit_untyped_atomic(), emit_typed_read(), emit_typed_write(), and emit_typed_atomic().<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>
+ /* Set the surface write address. */<br>
+ if (v->brw->is_haswell) {<br>
+ emit_assign_with_pad(make_mrf(mlen), addr, dims);<br>
+ mlen++;<br>
+ } else {<br>
+ emit_assign_to_transpose(make_mrf(mlen), addr, dims);<br>
+ mlen += dims;<br>
+ }<br>
+<br>
+ /* Set the source value. */<br>
+ if (v->brw->is_haswell) {<br>
+ emit_assign_with_pad(make_mrf(mlen), src, size);<br>
+ mlen++;<br>
+ } else {<br>
+ emit_assign_to_transpose(make_mrf(mlen), src, size);<br>
+ mlen += size;<br>
+ }<br>
+<br>
+ /* Emit the instruction. Note that this is translated into the<br>
+ * SIMD8 untyped surface write message on IVB because the<br>
+ * hardware lacks a SIMD4x2 counterpart.<br>
+ */<br>
+ vec4_instruction &inst = exec_predicated(<br>
+ flag, emit(SHADER_OPCODE_UNTYPED_SURFACE_WRITE,<br>
+ brw_writemask(brw_null_reg(), mask),<br>
+ surface, size));<br>
+ inst.base_mrf = 0;<br>
+ inst.mlen = mlen;<br>
+}<br>
+<br>
+backend_reg<br>
+brw_vec4_surface_visitor::emit_untyped_atomic(<br>
+ backend_reg flag, backend_reg surface, backend_reg addr,<br>
+ backend_reg src0, backend_reg src1,<br>
+ unsigned dims, unsigned op) const<br>
+{<br>
+ src_reg dst = make_grf(BRW_REGISTER_TYPE_UD, 1);<br>
+ unsigned mlen = 0;<br>
+<br>
+ /* Set the atomic operation address. */<br>
+ if (v->brw->is_haswell) {<br>
+ emit_assign_with_pad(make_mrf(mlen), addr, dims);<br>
+ mlen++;<br>
+ } else {<br>
+ emit_assign_to_transpose(make_mrf(mlen), addr, dims);<br>
+ mlen += dims;<br>
+ }<br>
+<br>
+ /* Set the source arguments. */<br>
+ if (v->brw->is_haswell) {<br>
+ if (src0.file != BAD_FILE)<br>
+ emit(BRW_OPCODE_MOV, writemask(make_mrf(mlen), WRITEMASK_X),<br>
+ src0);<br>
+<br>
+ if (src1.file != BAD_FILE)<br>
+ emit(BRW_OPCODE_MOV, writemask(make_mrf(mlen), WRITEMASK_Y),<br>
+ swizzle(src1, BRW_SWIZZLE_XXXX));<br>
+<br>
+ mlen++;<br></blockquote><div><br></div><div>According to Graphics BSpec: 3D-Media-GPGPU Engine > Shared Functions > Shared Functions – Data Port [Pre-SKL] > Messages > TypedUntyped Surface ReadWrite and TypedUntyped Atomic Operation [IVB+] > Message Payload > SIMD4x2 Source Payload (Atomic Operation message only), "The following atomic operations require no sources, thus the source payload is not delivered: AOP_INC, AOP_DEC, AOP_PREDEC".<br>
<br></div><div>I think this means that if both src0.file and src1.file are BAD_FILE, we shouldn't increment mlen.<br><br></div><div>A similar comment applies to emit_typed_atomic().<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>
+ } else {<br>
+ if (src0.file != BAD_FILE) {<br>
+ emit(BRW_OPCODE_MOV, writemask(make_mrf(mlen), WRITEMASK_X),<br>
+ src0);<br>
+ mlen++;<br>
+ }<br>
+<br>
+ if (src1.file != BAD_FILE) {<br>
+ emit(BRW_OPCODE_MOV, writemask(make_mrf(mlen), WRITEMASK_X),<br>
+ src1);<br>
+ mlen++;<br>
+ }<br>
+ }<br>
+<br>
+ /* Emit the instruction. Note that this is translated into the<br>
+ * SIMD8 untyped atomic message on IVB because the hardware lacks a<br>
+ * SIMD4x2 counterpart.<br>
+ */<br>
+ vec4_instruction &inst = exec_predicated(<br>
+ flag, emit(SHADER_OPCODE_UNTYPED_ATOMIC,<br>
+ writemask(dst, WRITEMASK_X),<br>
+ surface, op));<br>
+ inst.base_mrf = 0;<br>
+ inst.mlen = mlen;<br>
+<br>
+ return dst;<br>
+}<br>
+<br>
+backend_reg<br>
+brw_vec4_surface_visitor::emit_typed_read(<br>
+ backend_reg flag, backend_reg surface, backend_reg addr,<br>
+ unsigned dims, unsigned size) const<br>
+{<br>
+ const unsigned rlen = size * (v->brw->is_haswell ? 1 : 8);<br></blockquote><div><br></div><div>I think this is 2x larger than necessary for Ivy Bridge. brw_vec4_surface_visitor::make_grf() will divide rlen by 4 to determine how many registers to allocate. So, for example, if size == 4, it will allocate 8 consecutive registers to hold the result. But the reply from the untyped read message will only fill up 4 of them, and the call to emit_assign_from_transpose() will only cause 4 of them to be read. <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">
+backend_reg<br>
+brw_vec4_surface_visitor::emit_pack_generic(<br>
+ backend_reg src,<br>
+ unsigned shift_r, unsigned width_r,<br>
+ unsigned shift_g, unsigned width_g,<br>
+ unsigned shift_b, unsigned width_b,<br>
+ unsigned shift_a, unsigned width_a) const<br>
+{<br>
+ const unsigned mask = (!!width_r << 0 | !!width_g << 1 |<br>
+ !!width_b << 2 | !!width_a << 3);<br>
+ const bool homogeneous = ((!width_g || width_r == width_g) &&<br>
+ (!width_b || width_g == width_b) &&<br>
+ (!width_a || width_b == width_a));<br>
+ const unsigned bits = width_r + width_g + width_b + width_a;<br>
+ src_reg shift = make_grf(BRW_REGISTER_TYPE_UD, 4);<br>
+<br>
+ /* Shift left to discard the most significant bits. */<br>
+ emit(BRW_OPCODE_MOV, writemask(shift, mask),<br>
+ (homogeneous ? brw_imm_ud(32 - width_r) :<br>
+ brw_imm_vf4(32 - width_r, 32 - width_g,<br>
+ 32 - width_b, 32 - width_a)));<br></blockquote><div><br></div><div>It seems a little silly to go to extra effort to switch between brw_imm_ud() and brw_imm_vc4(), given that SHL and SHR only pay attention to the bottom 5 bits of their shift argument, and brw_imm_vf4 is capable of representing values up to 31. How about instead:<br>
<br> emit(BRW_OPCODE_MOV, writemask(shift, mask),<br> brw_imm_vf4((32 - width_r) % 32, (32 - width_g) % 32,<br> (32 - width_b) % 32, (32 - width_a) % 32));<br><br></div><div>A similar comment applies to emit_unpack_generic().<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>
+ emit(BRW_OPCODE_SHL, writemask(src, mask), src, shift);<br></blockquote><div><br><div>As in my comments on brw_fs_surface_visitor.cpp, I don't like writing to src.
It seems like there's too big a risk that a user of this function will
assume that src isn't written to (and can thus be used again). I'd
prefer to allocate a new temporary and then let register coalescing take
care of getting rid of it.<br></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>
+ /* Shift right to the final bit field positions. */<br>
+ emit(BRW_OPCODE_MOV, writemask(shift, mask),<br>
+ brw_imm_vf4(32 - shift_r % 32 - width_r,<br>
+ 32 - shift_g % 32 - width_g,<br>
+ 32 - shift_b % 32 - width_b,<br>
+ 32 - shift_a % 32 - width_a));<br></blockquote><div><br></div><div>Won't this have trouble with unused channels? For example, if shift_a == width_a == 0 (i.e. because channel a doesn't exist in the format we're packing), then we'll pass 32 to brw_imm_vf4(), which isn't allowed.<br>
<br></div><div>I think we need to do:<br><br> emit(BRW_OPCODE_MOV, writemask(shift, mask),<br> brw_imm_vf4((32 - shift_r % 32 - width_r) % 32,<br> (32 - shift_g % 32 - width_g) % 32,<br> (32 - shift_b % 32 - width_b) % 32,<br>
(32 - shift_a % 32 - width_a) % 32));<br><br></div><div>A similar comment applies to emit_unpack_generic().<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>
+ emit(BRW_OPCODE_SHR, writemask(src, mask), src, shift);<br>
+<br>
+ /* Add everything up. */<br></blockquote><div><br></div><div>Since we're not adding, let's change the comment to "OR everything together."<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">
+ if (mask >> 2)<br>
+ emit(BRW_OPCODE_OR,<br>
+ writemask(src, WRITEMASK_XY),<br>
+ swizzle(src, BRW_SWIZZLE_XZXZ),<br>
+ swizzle(src, (mask >> 3 ? BRW_SWIZZLE_YWYW :<br>
+ BRW_SWIZZLE_YZYZ)));<br>
+<br>
+ if (mask >> 1 && bits <= 32)<br>
+ emit(BRW_OPCODE_OR,<br>
+ writemask(src, WRITEMASK_X),<br>
+ swizzle(src, BRW_SWIZZLE_XXXX),<br>
+ swizzle(src, BRW_SWIZZLE_YYYY));<br></blockquote><div><br></div>This code only works if a number of assumptions about the shift and width values are correct. Can we document (and check) those assumptions with assertions? E.g. something like:<br>
<br> assert(shift_r + width_r <= 32);<br> if (bits <= 32) {<br> // Pack all channels into result.x<br> assert(!width_g || shift_g + width_g <= 32);<br> assert(!width_b || shift_b + width_b <= 32);<br>
assert(!width_a || shift_a + width_a <= 32);<br> } else if (mask >> 2) {<br> // Pack rg into result.x and bw into result.y<br> assert(shift_g + width_g <= 32);<br> assert(width_b >= 32 && shift_b + width_b <= 64);<br>
assert(!width_a || (width_a >= 32 && shift_a + width_a <= 64));<br> } else {<br> // Pack r into result.x and g into result.y<br> assert(width_g >= 32 &7 shift_g + width_g <= 64);<br>
}<br><br></div><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">
+<br>
+ return src;<br>
+}<br>
+<br>
+backend_reg<br>
+brw_vec4_surface_visitor::emit_unpack_generic(<br>
+ backend_reg src,<br>
+ unsigned shift_r, unsigned width_r,<br>
+ unsigned shift_g, unsigned width_g,<br>
+ unsigned shift_b, unsigned width_b,<br>
+ unsigned shift_a, unsigned width_a) const<br>
+{<br>
+ const unsigned mask = (!!width_r << 0 | !!width_g << 1 |<br>
+ !!width_b << 2 | !!width_a << 3);<br>
+ const bool homogeneous = ((!width_g || width_r == width_g) &&<br>
+ (!width_b || width_g == width_b) &&<br>
+ (!width_a || width_b == width_a));<br>
+ src_reg shift = make_grf(BRW_REGISTER_TYPE_UD, 4);<br>
+ src_reg dst = make_grf(src.type, 4);<br>
+<br>
+ /* Shift left to discard the most significant bits. */<br>
+ emit(BRW_OPCODE_MOV, writemask(shift, mask),<br>
+ brw_imm_vf4(32 - shift_r % 32 - width_r,<br>
+ 32 - shift_g % 32 - width_g,<br>
+ 32 - shift_b % 32 - width_b,<br>
+ 32 - shift_a % 32 - width_a));<br>
+<br>
+ emit(BRW_OPCODE_SHL, writemask(dst, mask),<br>
+ swizzle(src, BRW_SWIZZLE4(shift_r / 32, shift_g / 32,<br>
+ shift_b / 32, shift_a / 32)),<br>
+ shift);<br>
+<br>
+ /* Shift back to the least significant bits using an arithmetic<br>
+ * shift to get sign extension on signed types.<br>
+ */<br>
+ emit(BRW_OPCODE_MOV, writemask(shift, mask),<br>
+ (homogeneous ? brw_imm_ud(32 - width_r) :<br>
+ brw_imm_vf4(32 - width_r, 32 - width_g,<br>
+ 32 - width_b, 32 - width_a)));<br>
+<br>
+ emit(BRW_OPCODE_ASR, writemask(dst, mask), dst, shift);<br>
+<br>
+ return dst;<br>
+}<br>
+<br>
+backend_reg<br>
+brw_vec4_surface_visitor::emit_pack_homogeneous(<br>
+ backend_reg src,<br>
+ unsigned shift_r, unsigned width_r,<br>
+ unsigned shift_g, unsigned width_g,<br>
+ unsigned shift_b, unsigned width_b,<br>
+ unsigned shift_a, unsigned width_a) const<br>
+{<br>
+ /* We could do the same with less instructions if we had some way<br>
+ * to use Align1 addressing in the VEC4 visitor. Just use the<br>
+ * general path for now...<br>
+ */<br>
+ return emit_pack_generic(src, shift_r, width_r, shift_g, width_g,<br>
+ shift_b, width_b, shift_a, width_a);<br>
+}<br>
+<br>
+backend_reg<br>
+brw_vec4_surface_visitor::emit_unpack_homogeneous(<br>
+ backend_reg src,<br>
+ unsigned shift_r, unsigned width_r,<br>
+ unsigned shift_g, unsigned width_g,<br>
+ unsigned shift_b, unsigned width_b,<br>
+ unsigned shift_a, unsigned width_a) const<br>
+{<br>
+ /* We could do the same with less instructions if we had some way<br>
+ * to use Align1 addressing in the VEC4 visitor. Just use the<br>
+ * general path for now...<br>
+ */<br>
+ return emit_unpack_generic(src, shift_r, width_r, shift_g, width_g,<br>
+ shift_b, width_b, shift_a, width_a);<br>
+}<br></blockquote><div><br></div><div>It seems a little odd that for fs, the generic and homogeneous packing functions are completely separate; but for vec4, they wind up calling the same function, which then dynamically determines whether or not we're doing homogeneous packing/unpacking.<br>
<br></div><div>How about if instead we simplify the interface so that there's just an emit_pack() function and an emit_unpack() function. In the vec4 implementation, we can just drop the emit_{pack,unpack}_homogeneous() functions and rename emit_{pack,unpack}_generic() to emit_{pack,unpack}. In the fs implementation, we can do:<br>
<br></div><div>brw_fs_surface_visitor::emit_{pack,unpack}(...)<br>{<br> const bool homogeneous = ((!width_g || width_r == width_g) &&<br> (!width_b || width_g == width_b) &&<br>
(!width_a || width_b == width_a));<br></div><div> if (homogeneous) {<br></div><div> ...homogeneous logic...<br></div><div> } else {<br></div><div> ...generic logic...<br> }<br>
}<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>
+backend_reg<br>
+brw_vec4_surface_visitor::emit_convert_to_float(<br>
+ backend_reg src,<br>
+ unsigned mask0, unsigned width0,<br>
+ unsigned mask1, unsigned width1) const<br></blockquote><div><br></div><div>My comments about the fs implementation of emit_convert_to_float() apply here as well.<br></div><div> </div><br></div><div class="gmail_quote">
Final thoughts about this patch, now that I've gotten through it:<br><br></div><div class="gmail_quote">1. The patch is very large, and that made reviewing it difficult. Please break it out into separate patches. One possible way of breaking it up would be: a. add the brw_surface_visitor base class. b. Add the brw_fs_surface_visitor class. c. Add the brw_vec4_surface_visitor class. That would have the added advantage that it would lead people to review the base class before reviewing the derived classes, which I think would have been an easier order to review the patch in.<br>
<br></div><div class="gmail_quote">2. The object splicing concerns I brought up in "[PATCH 06/23] i965: Define common register base class shared between both back-ends." apply to this patch as well. I will try to talk to some folks at Intel next week about my ideas to mitigate this.<br>
</div></div></div>