<html><head></head><body><div>On Wed, 2018-04-25 at 07:05 -0700, Jason Ekstrand wrote:</div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr">Some of these comments may be duplicates of ones I made the first time through.<br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Apr 11, 2018 at 12:20 AM, Iago Toral Quiroga <span dir="ltr"><<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>></span> wrote:<br><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">The hardware doesn't support 16-bit integer types, so we need to implement<br>
these using 32-bit integer instructions and then convert the result back<br>
to 16-bit.<br>
---<br>
src/intel/Makefile.sources | 1 +<br>
src/intel/compiler/brw_nir.c | 2 +<br>
src/intel/compiler/brw_nir.h | 2 +<br>
src/intel/compiler/brw_nir_lower_16bit_int_math.c | 108 ++++++++++++++++++++++<br>
src/intel/compiler/meson.build | 1 +<br>
5 files changed, 114 insertions(+)<br>
create mode 100644 src/intel/compiler/brw_nir_lower_16bit_int_math.c<br>
<br>
diff --git a/src/intel/Makefile.sources b/src/intel/Makefile.sources<br>
index 91c71a8dfaf..2cd76961ea4 100644<br>
--- a/src/intel/Makefile.sources<br>
+++ b/src/intel/Makefile.sources<br>
@@ -79,6 +79,7 @@ COMPILER_FILES = \<br>
compiler/brw_nir_analyze_boolean_resolves.c \<br>
compiler/brw_nir_analyze_ubo_ranges.c \<br>
compiler/brw_nir_attribute_workarounds.c \<br>
+ compiler/brw_nir_lower_16bit_int_math.c \<br>
compiler/brw_nir_lower_cs_intrinsics.c \<br>
compiler/brw_nir_opt_peephole_ffma.c \<br>
compiler/brw_nir_tcs_workarounds.c \<br>
diff --git a/src/intel/compiler/brw_nir.c b/src/intel/compiler/brw_nir.c<br>
index 69ab162f888..2e5754076ed 100644<br>
--- a/src/intel/compiler/brw_nir.c<br>
+++ b/src/intel/compiler/brw_nir.c<br>
@@ -638,6 +638,8 @@ brw_preprocess_nir(const struct brw_compiler *compiler, nir_shader *nir)<br>
nir_lower_isign64 |<br>
nir_lower_divmod64);<br>
<br>
+ brw_nir_lower_16bit_int_math(nir);<br>
+<br>
nir = brw_nir_optimize(nir, compiler, is_scalar);<br>
<br>
if (is_scalar) {<br>
diff --git a/src/intel/compiler/brw_nir.h b/src/intel/compiler/brw_nir.h<br>
index 03f52da08e5..6ba1a8bc654 100644<br>
--- a/src/intel/compiler/brw_nir.h<br>
+++ b/src/intel/compiler/brw_nir.h<br>
@@ -152,6 +152,8 @@ void brw_nir_analyze_ubo_ranges(const struct brw_compiler *compiler,<br>
<br>
bool brw_nir_opt_peephole_ffma(nir_shader *shader);<br>
<br>
+bool brw_nir_lower_16bit_int_math(nir_shader *shader);<br>
+<br>
nir_shader *brw_nir_optimize(nir_shader *nir,<br>
const struct brw_compiler *compiler,<br>
bool is_scalar);<br>
diff --git a/src/intel/compiler/brw_nir_lower_16bit_int_math.c b/src/intel/compiler/brw_nir_lower_16bit_int_math.c<br>
new file mode 100644<br>
index 00000000000..6876309a822<br>
--- /dev/null<br>
+++ b/src/intel/compiler/brw_nir_lower_16bit_int_math.c<br>
@@ -0,0 +1,108 @@<br>
+/*<br>
+ * Copyright © 2018 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>
+<br>
+#include "brw_nir.h"<br>
+#include "nir_builder.h"<br>
+<br>
+/**<br>
+ * Intel hardware doesn't support 16-bit integer Math instructions so this<br>
+ * pass implements them in 32-bit and then converts the result back to 16-bit.<br>
+ */<br>
+static void<br>
+lower_math_instr(nir_builder *bld, nir_alu_instr *alu, bool is_signed)<br>
+{<br>
+ const nir_op op = alu->op;<br>
+<br>
+ bld->cursor = nir_before_instr(&alu->instr);<br>
+<br>
+ nir_ssa_def *srcs_32[4] = { NULL, NULL, NULL, NULL };<br>
+ const uint32_t num_inputs = nir_op_infos[op].num_inputs;<br>
+ for (uint32_t i = 0; i < num_inputs; i++) {<br>
+ nir_ssa_def *src = nir_ssa_for_alu_src(bld, alu, i);<br>
+ srcs_32[i] = is_signed ? nir_i2i32(bld, src) : nir_u2u32(bld, src);<br></blockquote><div><br></div><div>For float16, we'll need f2f32. </div></div></div></div></blockquote><div><br></div><div>Yes, I have that (in a separate pass for float16), I suppose merging both makes more sense that having them be separate.</div><div><br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> Also, is_signed can be derived from nir_op_infos[op].input_types so it doesn't need to be passed in. If we want to make it fully general, we probably also want to only do the conversion if the source type is unsized.<br></div></div></div></div></blockquote><div><br></div><div>Good point.</div><div><br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
+ }<br>
+<br>
+ nir_ssa_def *dst_32 =<br>
+ nir_build_alu(bld, op, srcs_32[0], srcs_32[1], srcs_32[2], srcs_32[3]);<br>
+<br>
+ nir_ssa_def *dst_16 =<br>
+ is_signed ? nir_i2i16(bld, dst_32) : nir_u2u16(bld, dst_32);<br></blockquote><div><br></div><div>Again, we can pull this from the destination type.<br></div><div> </div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
+<br>
+ nir_ssa_def_rewrite_uses(&alu->dest.dest.ssa, nir_src_for_ssa(dst_16));<br>
+}<br>
+<br>
+static bool<br>
+lower_instr(nir_builder *bld, nir_alu_instr *alu)<br>
+{<br></blockquote><div><br></div><div>As mentioned in previous discussions, we may want to have a control function such as<br><br></div><div>unsigned (*lower_bit_size)(const nir_alu_instr *, void *)<br><br></div><div>where the void * is something you pass in when you call the optimization pass. I'm usually not a huge fan of making a super-general thing before we need it. However, we already have two different drivers that need it for different things so let's just do it, make sure it works for all the use-cases, and get it right the first time.<br></div></div></div></div></blockquote><div><br></div><div>Yes, I agree.</div><div><br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
+ assert(alu->dest.dest.is_ssa);<br>
+ if (alu->dest.dest.ssa.bit_size != 16)<br>
+ return false;<br>
+<br>
+ bool is_signed = false;<br>
+ switch (alu->op) {<br>
+ case nir_op_idiv:<br>
+ case nir_op_imod:<br>
+ is_signed = true;<br>
+ /* Fallthrough */<br>
+ case nir_op_udiv:<br>
+ case nir_op_umod:<br>
+ case nir_op_irem:<br></blockquote><div><br></div><div>irem is sgned.<br></div></div></div></div></blockquote><div><br></div><div>Oops, right.</div><div><br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
+ lower_math_instr(bld, alu, is_signed);<br>
+ return true;<br>
+ default:<br>
+ return false;<br>
+ }<br>
+}<br>
+<br>
+static bool<br>
+lower_impl(nir_function_impl *impl)<br>
+{<br>
+ nir_builder b;<br>
+ nir_builder_init(&b, impl);<br>
+ bool progress = false;<br>
+<br>
+ nir_foreach_block(block, impl) {<br>
+ nir_foreach_instr_safe(instr, block) {<br>
+ if (instr->type == nir_instr_type_alu)<br>
+ progress |= lower_instr(&b, nir_instr_as_alu(instr));<br>
+ }<br>
+ }<br>
+<br>
+ nir_metadata_preserve(impl, nir_metadata_block_index |<br>
+ nir_metadata_dominance);<br></blockquote><div><br></div><div>Probably only want to call this if (progress)<br></div></div></div></div></blockquote><div><br></div><div>Ok.</div><div><br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
+<br>
+ return progress;<br>
+}<br>
+<br>
+bool<br>
+brw_nir_lower_16bit_int_math(nir_shader *shader)<br></blockquote><div><br></div><div>If we want this to handle 8-bit things, maybe it needs a different name. :-)<br></div></div></div></div></blockquote><div><br></div><div>Yes, and also if we want this to handle floats.</div><div><br></div><div>Iago</div><div><br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
+{<br>
+ bool progress = false;<br>
+<br>
+ nir_foreach_function(function, shader) {<br>
+ if (function->impl)<br>
+ progress |= lower_impl(function->impl);<br>
+ }<br>
+<br>
+ return progress;<br>
+}<br>
diff --git a/src/intel/compiler/meson.build b/src/intel/compiler/meson.build<br>
index 72b7a6796cb..d80fcd6e31b 100644<br>
--- a/src/intel/compiler/meson.build<br>
+++ b/src/intel/compiler/meson.build<br>
@@ -76,6 +76,7 @@ libintel_compiler_files = files(<br>
'brw_nir_analyze_boolean_resolves.c',<br>
'brw_nir_analyze_ubo_ranges.c',<br>
'brw_nir_attribute_workarounds.c',<br>
+ 'brw_nir_lower_16bit_int_math.c',<br>
'brw_nir_lower_cs_intrinsics.c',<br>
'brw_nir_opt_peephole_ffma.c',<br>
'brw_nir_tcs_workarounds.c',<br>
<span class="HOEnZb"><font color="#888888">-- <br>
2.14.1<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>
</blockquote></body></html>