<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Mon, Dec 10, 2018 at 10:28 PM Timothy Arceri <<a href="mailto:tarceri@itsqueeze.com">tarceri@itsqueeze.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">If we are outputting the same value to more than one output<br>
component rewrite the inputs to read from a single component.<br>
<br>
This will allow the duplicate varying components to be optimised<br>
away by the existing opts.<br>
<br>
shader-db results i965 (SKL):<br>
<br>
total instructions in shared programs: 12869230 -> 12860886 (-0.06%)<br>
instructions in affected programs: 322601 -> 314257 (-2.59%)<br>
helped: 3080<br>
HURT: 8<br>
<br>
total cycles in shared programs: 317792574 -> 317730593 (-0.02%)<br>
cycles in affected programs: 2584925 -> 2522944 (-2.40%)<br>
helped: 2975<br>
HURT: 477<br>
<br>
shader-db results radeonsi (VEGA):<br>
<br>
Totals from affected shaders:<br>
SGPRS: 30960 -> 31056 (0.31 %)<br>
VGPRS: 17052 -> 16672 (-2.23 %)<br>
Spilled SGPRs: 184 -> 167 (-9.24 %)<br>
Spilled VGPRs: 0 -> 0 (0.00 %)<br>
Private memory VGPRs: 0 -> 0 (0.00 %)<br>
Scratch size: 0 -> 0 (0.00 %) dwords per thread<br>
Code Size: 562532 -> 549404 (-2.33 %) bytes<br>
LDS: 0 -> 0 (0.00 %) blocks<br>
Max Waves: 6011 -> 6110 (1.65 %)<br>
Wait states: 0 -> 0 (0.00 %)<br>
<br>
vkpipeline-db results RADV (VEGA):<br>
<br>
Totals from affected shaders:<br>
SGPRS: 14880 -> 15080 (1.34 %)<br>
VGPRS: 10872 -> 10888 (0.15 %)<br>
Spilled SGPRs: 0 -> 0 (0.00 %)<br>
Spilled VGPRs: 0 -> 0 (0.00 %)<br>
Private memory VGPRs: 0 -> 0 (0.00 %)<br>
Scratch size: 0 -> 0 (0.00 %) dwords per thread<br>
Code Size: 674016 -> 668396 (-0.83 %) bytes<br>
LDS: 0 -> 0 (0.00 %) blocks<br>
Max Waves: 2708 -> 2704 (-0.15 %)<br>
Wait states: 0 -> 0 (0.00 %<br>
---<br>
src/compiler/nir/nir_linking_helpers.c | 95 ++++++++++++++++++++++++++<br>
1 file changed, 95 insertions(+)<br>
<br>
diff --git a/src/compiler/nir/nir_linking_helpers.c b/src/compiler/nir/nir_linking_helpers.c<br>
index 37644d339f..bdfa7b8c4d 100644<br>
--- a/src/compiler/nir/nir_linking_helpers.c<br>
+++ b/src/compiler/nir/nir_linking_helpers.c<br>
@@ -700,6 +700,27 @@ nir_link_xfb_varyings(nir_shader *producer, nir_shader *consumer)<br>
}<br>
}<br>
<br>
+static nir_variable *<br>
+get_matching_input_var(nir_shader *consumer, nir_variable *out_var)<br>
+{<br>
+ nir_variable *input_var = NULL;<br>
+ nir_foreach_variable(var, &consumer->inputs) {<br>
+ if (var->data.location >= VARYING_SLOT_VAR0 &&<br>
+ var->data.location - VARYING_SLOT_VAR0 < MAX_VARYING) {<br></blockquote><div><br></div><div>Why is this not redundant with the checks below? If it's not redundant, I'd add a comment and make this one a continue because it's obviously skipping over something. Looking at patch 3 and the current code, I'm very sure this isn't actually needed. Maybe just assert that the above is true of out_var->data.location and just let the check below handle it for var?<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 (var->data.location == out_var->data.location &&<br>
+ var->data.location_frac == out_var->data.location_frac &&<br>
+ var->data.interpolation == out_var->data.interpolation &&<br>
+ get_interp_loc(var) == get_interp_loc(out_var)) {<br>
+ input_var = var;<br></blockquote><div><br></div>return var; ?<br></div><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ break;<br>
+ }<br>
+ }<br>
+ }<br>
+<br>
+ return input_var;<br>
+}<br>
+<br>
static bool<br>
can_replace_varying(nir_variable *out_var, nir_intrinsic_instr *store_intr)<br>
{<br>
@@ -782,6 +803,57 @@ try_replace_constant_input(nir_shader *shader,<br>
return progress;<br>
}<br>
<br>
+static bool<br>
+try_replace_duplicate_input(nir_shader *shader, nir_variable *input_var,<br>
+ nir_intrinsic_instr *dup_store_intr)<br>
+{<br>
+ assert(input_var);<br>
+<br>
+ nir_variable *dup_out_var =<br>
+ nir_deref_instr_get_variable(nir_src_as_deref(dup_store_intr->src[0]));<br>
+<br>
+ if (!can_replace_varying(dup_out_var, dup_store_intr))<br>
+ return false;<br></blockquote><div><br></div><div>This isn't much of a "try" function if it's just got one single check at the top.<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>
+ nir_function_impl *impl = nir_shader_get_entrypoint(shader);<br>
+<br>
+ nir_builder b;<br>
+ nir_builder_init(&b, impl);<br>
+<br>
+ bool progress = false;<br>
+ nir_foreach_block(block, impl) {<br>
+ nir_foreach_instr(instr, block) {<br>
+ if (instr->type != nir_instr_type_intrinsic)<br>
+ continue;<br>
+<br>
+ nir_intrinsic_instr *intr = nir_instr_as_intrinsic(instr);<br>
+ if (intr->intrinsic != nir_intrinsic_load_deref)<br>
+ continue;<br>
+<br>
+ nir_variable *in_var =<br>
+ nir_deref_instr_get_variable(nir_src_as_deref(intr->src[0]));<br></blockquote><div><br></div><div>Please do something like this instead:</div><div><br></div><div>nir_deref_instr *in_deref = nir_src_as_deref(instr->src[0]);</div><div>if (in_deref->mode != nir_var_shader_in)</div><div> continue;</div><div><br></div><div>nir_variable *in_var = nir_deref_instr_get_variable(in_deref);<br></div><div><br></div><div>By checking the mode on the deref instead of the variable, the pass will work even in the presence of derefs which you can't chase back to the variable as long as they aren't on an input. For some of my UBO/SSBO deref stuff, I've got patches that make exactly this change for about four other linking passes.<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 (in_var->data.mode != nir_var_shader_in)<br>
+ continue;<br>
+<br>
+ if (in_var->data.location != dup_out_var->data.location ||<br>
+ in_var->data.location_frac != dup_out_var->data.location_frac ||<br>
+ in_var->data.interpolation != input_var->data.interpolation ||<br>
+ get_interp_loc(in_var) != get_interp_loc(input_var))<br>
+ continue;<br>
+<br>
+ b.cursor = nir_before_instr(instr);<br>
+<br>
+ nir_ssa_def *load = nir_load_var(&b, input_var);<br>
+ nir_ssa_def_rewrite_uses(&intr->dest.ssa, nir_src_for_ssa(load));<br>
+<br>
+ progress = true;<br>
+ }<br>
+ }<br>
+<br>
+ return progress;<br>
+}<br>
+<br>
bool<br>
nir_link_opt_varyings(nir_shader *producer, nir_shader *consumer)<br>
{<br>
@@ -795,6 +867,10 @@ nir_link_opt_varyings(nir_shader *producer, nir_shader *consumer)<br>
<br>
nir_function_impl *impl = nir_shader_get_entrypoint(producer);<br>
<br>
+ struct hash_table *varying_values =<br>
+ _mesa_hash_table_create(NULL, _mesa_hash_pointer,<br>
+ _mesa_key_pointer_equal);<br>
+<br>
/* If we find a store in the last block of the producer we can be sure this<br>
* is the only possible value for this output.<br>
*/<br>
@@ -809,11 +885,30 @@ nir_link_opt_varyings(nir_shader *producer, nir_shader *consumer)<br>
continue;<br></blockquote><div><br></div><div>In order to sort out the deref problems, I think you want something like this here:</div><div><br></div><div><div>nir_deref_instr *out_deref = nir_src_as_deref(instr->src[0]);</div><div>if (out_deref->mode != nir_var_shader_out)</div><div> continue;</div><div><br></div><div>I also can't help the feeling that this whole block below feels a bit sloppy to me. I think the whole mess would be greatly improved if, now that we have can_replace_varying, we just did this here:</div><div><br></div><div>nir_variable *out_var = nir_deref_instr_get_variable(out_deref);<br></div><div>if (!can_replace_varying(out_var))</div><div> continue;</div><div><br></div><div>Then we could get rid of the can_replace_varying call below; replace_constant_input is no longer a "try" and replace_duplicate_input doesn't have to be a "try" either.<br></div></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 (intr->src[1].ssa->parent_instr->type != nir_instr_type_load_const) {<br>
+ struct hash_entry *entry =<br>
+ _mesa_hash_table_search(varying_values, intr->src[1].ssa);<br>
+ if (entry) {<br>
+ progress |=<br>
+ try_replace_duplicate_input(consumer,<br>
+ (nir_variable *) entry->data,<br>
+ intr);<br>
+ } else {<br>
+ nir_variable *out_var =<br>
+ nir_deref_instr_get_variable(nir_src_as_deref(intr->src[0]));<br></blockquote><div><br></div><div>This is also going to blow up if nir_deref_instr_get_variable returns NULL. See just 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">
+ nir_variable *in_var = get_matching_input_var(consumer, out_var);<br>
+ if (in_var && can_replace_varying(out_var, intr)) {<br>
+ _mesa_hash_table_insert(varying_values, intr->src[1].ssa,<br>
+ in_var);<br>
+ }<br>
+ }<br>
+<br>
continue;<br>
}<br>
<br>
progress |= try_replace_constant_input(consumer, intr);<br></blockquote><div><br></div><div>Assuming the can_replace_varying() continue above, I think this would be much cleaner as:</div><div><br></div><div>if (nir_src_is_const(instr->src[1])) {</div><div> replace_constant_input(consumer, intr);</div><div> progress = true;</div><div> continue;<br></div><div>}</div><div><br></div><div>entry = _mesa_hash_table_search(varying_values, intr->src[1].ssa);</div><div>if (entry) {</div><div> replace_duplicate_input(consumer, entry->data, intr);</div><div> progress = true;</div><div> continue;</div><div>} else {</div><div> nir_variable *in_var = get_matching_input_var(consumer, out_var);</div><div> if (in_var)</div><div> _mesa_hash_table_insert(varying_values, intr->src[1].ssa, in_var);<br></div><div>}</div><div><br></div><div>Is all this actually better or am I just making hash of things for no reason? I seriously did not intend to leave that many comments. I'm sorry. :-(</div><div><br></div><div>--Jason<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>
+ _mesa_hash_table_destroy(varying_values, NULL);<br>
+<br>
return progress;<br>
}<br>
-- <br>
2.19.2<br>
<br>
_______________________________________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">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/mailman/listinfo/mesa-dev</a><br>
</blockquote></div></div>