<div dir="ltr"><div>I've left some comments below that I think clean things up and make this better, but I believe it is correct as-is.<br><br></div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 29, 2017 at 6:42 PM, Jose Maria Casanova Crespo <span dir="ltr"><<a href="mailto:jmcasanova@igalia.com" target="_blank">jmcasanova@igalia.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">From: Alejandro Piñeiro <<a href="mailto:apinheiro@igalia.com">apinheiro@igalia.com</a>><br>
<br>
We need to rely on byte scattered writes as untyped writes are 32-bit<br>
size. We could try to keep using 32-bit messages when we have two or<br>
four 16-bit elements, but for simplicity sake, we use the same message<br>
for any component number. We revisit this aproach in the follwing<br>
patches.<br>
<br>
v2: Removed use of stride = 2 on 16-bit sources (Jason Ekstrand)<br>
<br>
v3: (Jason Ekstrand)<br>
- Include bit_size to scattered write message and remove namespace<br>
- specific for scattered messages.<br>
- Move comment to proper place.<br>
- Squashed with i965/fs: Adjust type_size/type_slots on store_ssbo.<br>
(Jose Maria Casanova)<br>
- Take into account that get_nir_src returns now WORD types for<br>
16-bit sources instead of DWORD.<br>
<br>
Signed-off-by: Jose Maria Casanova Crespo <<a href="mailto:jmcasanova@igalia.com">jmcasanova@igalia.com</a>><br>
Signed-off-by: Alejandro Piñeiro <<a href="mailto:apinheiro@igalia.com">apinheiro@igalia.com</a>><br>
---<br>
src/intel/compiler/brw_fs_nir.<wbr>cpp | 51 ++++++++++++++++++++++++++++--<wbr>---------<br>
1 file changed, 37 insertions(+), 14 deletions(-)<br>
<br>
diff --git a/src/intel/compiler/brw_fs_<wbr>nir.cpp b/src/intel/compiler/brw_fs_<wbr>nir.cpp<br>
index d6ab286147..ff04e2468b 100644<br>
--- a/src/intel/compiler/brw_fs_<wbr>nir.cpp<br>
+++ b/src/intel/compiler/brw_fs_<wbr>nir.cpp<br>
@@ -4075,14 +4075,15 @@ fs_visitor::nir_emit_<wbr>intrinsic(const fs_builder &bld, nir_intrinsic_instr *instr<br>
* Also, we have to suffle 64-bit data to be in the appropriate layout<br>
* expected by our 32-bit write messages.<br>
*/<br>
- unsigned type_size = 4;<br>
- if (nir_src_bit_size(instr->src[<wbr>0]) == 64) {<br>
- type_size = 8;<br>
+ unsigned bit_size = nir_src_bit_size(instr->src[0]<wbr>);<br>
+ unsigned type_size = bit_size / 8;<br>
+ if (bit_size == 64) {<br>
val_reg = shuffle_64bit_data_for_32bit_<wbr>write(bld,<br>
val_reg, instr->num_components);<br>
}<br>
<br>
- unsigned type_slots = type_size / 4;<br>
+ /* 16-bit types would use a minimum of 1 slot */<br>
+ unsigned type_slots = MAX2(type_size / 4, 1);<br></blockquote><div><br></div><div>Given that this is only used for emit_typed_write, maybe we should just move it next to the emit_typed_write call and just get rid of the MAX2(). More on that later.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
/* Combine groups of consecutive enabled channels in one write<br>
* message. We use ffs to find the first enabled channel and then ffs on<br>
@@ -4093,12 +4094,19 @@ fs_visitor::nir_emit_<wbr>intrinsic(const fs_builder &bld, nir_intrinsic_instr *instr<br>
unsigned first_component = ffs(writemask) - 1;<br>
unsigned length = ffs(~(writemask >> first_component)) - 1;<br></blockquote><div><br></div><div>If the one above is first_component, num_components would be a better name for this one. It's very confusing go have something generically named "length" in a piece of code with so many different possible units.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
- /* We can't write more than 2 64-bit components at once. Limit the<br>
- * length of the write to what we can do and let the next iteration<br>
- * handle the rest<br>
- */<br>
- if (type_size > 4)<br>
+ if (type_size > 4) {<br>
+ /* We can't write more than 2 64-bit components at once. Limit<br>
+ * the length of the write to what we can do and let the next<br>
+ * iteration handle the rest.<br>
+ */<br>
length = MIN2(2, length);<br>
+ } else if (type_size == 2) {<br></blockquote><div><br></div><div>Maybe type_size < 4?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ /* For 16-bit types we are using byte scattered writes, that can<br>
+ * only write one component per call. So we limit the length, and<br>
+ * let the write happening in several iterations.<br>
+ */<br>
+ length = 1;<br>
+ }<br>
<br>
fs_reg offset_reg;<br>
nir_const_value *const_offset = nir_src_as_const_value(instr-><wbr>src[2]);<br>
@@ -4112,11 +4120,26 @@ fs_visitor::nir_emit_<wbr>intrinsic(const fs_builder &bld, nir_intrinsic_instr *instr<br>
brw_imm_ud(type_size * first_component));<br>
}<br>
<br>
-<br>
- emit_untyped_write(bld, surf_index, offset_reg,<br>
- offset(val_reg, bld, first_component * type_slots),<br>
- 1 /* dims */, length * type_slots,<br>
- BRW_PREDICATE_NONE);<br>
+ if (type_size == 2) {<br></blockquote><div><br></div><div>maybe type_size < 4?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ /* Untyped Surface messages have a fixed 32-bit size, so we need<br>
+ * to rely on byte scattered in order to write 16-bit elements.<br>
+ * The byte_scattered_write message needs that every written 16-bit<br>
+ * type to be aligned 32-bits (stride=2).<br>
+ */<br>
+ fs_reg tmp = bld.vgrf(BRW_REGISTER_TYPE_D);<br>
+ bld.MOV(subscript(tmp, BRW_REGISTER_TYPE_W, 0),<br>
+ offset(val_reg, bld, first_component));<br>
+ emit_byte_scattered_write(bld, surf_index, offset_reg,<br>
+ tmp,<br>
+ 1 /* dims */, 1,<br>
+ bit_size,<br>
+ BRW_PREDICATE_NONE);<br>
+ } else {<br></blockquote><div><br></div><div>If we moved type_slots here, I think we could very nicely future-proof things as follows:</div><div><br></div><div>assert(num_components * type_size < 16);</div><div>assert((num_components * type_size) % 4 == 0);</div><div>assert((first_component * type_size) % 4 == 0);</div><div>unsigned first_slot = (first_component * type_size) / 4;</div><div>unsigned num_slots = (num_components * type_size) / 4;</div><div>emit_untyped_write(bld, surf_index, reg_offset,</div><div> offset(val_reg, bld, first_slot),</div><div> 1 /* dims */, num_slots,</div><div> BRW_PREDICATE_NONE);</div><div><br></div><div>That said, let's not get ahead of ourselves. This can all be done as a later clean-up on top of the optimization patch if that's easier. :)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ emit_untyped_write(bld, surf_index, offset_reg,<br>
+ offset(val_reg, bld, first_component * type_slots),<br>
+ 1 /* dims */, length * type_slots,<br>
+ BRW_PREDICATE_NONE);<br>
+ }<br>
<br>
/* Clear the bits in the writemask that we just wrote, then try<br>
* again to see if more channels are left.<br>
<span class="HOEnZb"><font color="#888888">--<br>
2.14.3<br>
<br>
______________________________<wbr>_________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-dev</a><br>
</font></span></blockquote></div><br></div></div>