<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Mon, Aug 27, 2018 at 4:09 AM Timothy Arceri <<a href="mailto:tarceri@itsqueeze.com" target="_blank">tarceri@itsqueeze.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Since we know what side of the branch we ended up on we can just replace the use with a constant. All the spill changes in shader-db are from Dolphin uber shaders, despite some small regressions the change is clearly positive. V2: insert new constant after any phis in the use->parent_instr->type == nir_instr_type_phi path. v3: - use nir_after_block_before_jump() for inserting const - check dominance of phi uses correctly v4: - create some helpers as suggested by Jason. shader-db results IVB: total instructions in shared programs: 9999201 -> 9993483 (-0.06%) instructions in affected programs: 163235 -> 157517 (-3.50%) helped: 132 HURT: 2 total cycles in shared programs: 231670754 -> 219476091 (-5.26%) cycles in affected programs: 143424120 -> 131229457 (-8.50%) helped: 115 HURT: 24 total spills in shared programs: 4383 -> 4370 (-0.30%) spills in affected programs: 1656 -> 1643 (-0.79%) helped: 9 HURT: 18 total fills in shared programs: 4610 -> 4581 (-0.63%) fills in affected programs: 374 -> 345 (-7.75%) helped: 6 HURT: 0 --- src/compiler/nir/nir.h | 22 +++++++ src/compiler/nir/nir_opt_if.c | 113 ++++++++++++++++++++++++++++++++++ 2 files changed, 135 insertions(+) diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h index 009a6d60371..0caacd30173 100644 --- a/src/compiler/nir/nir.h +++ b/src/compiler/nir/nir.h @@ -2331,6 +2331,28 @@ nir_after_block_before_jump(nir_block *block) } } +static inline nir_cursor +nir_before_src(nir_src *src, bool is_if_condition) +{ + if (is_if_condition) { + nir_block *prev_block = + nir_cf_node_as_block(nir_cf_node_prev(&src->parent_if->cf_node)); + assert(!nir_block_ends_in_jump(prev_block)); + return nir_after_block(prev_block); + } else if (src->parent_instr->type == nir_instr_type_phi) { + nir_phi_instr *cond_phi = nir_instr_as_phi(src->parent_instr); + nir_foreach_phi_src(phi_src, cond_phi) { + if (phi_src->src.ssa == src->ssa) { + return nir_after_block_before_jump(phi_src->pred); + } + } </blockquote><div> </div><div>This works. Some sort of container_of would be more efficient. I guess that wouldn't work if the source was a register indirect on a phi src but I don't think that's valid. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + + unreachable("failed to find phi src"); </blockquote><div> </div><div>If the source isn't a phi src (and container_of would fail) hitting an unreachable is really nasty. I don't think we will hit this case but I also think that if we do, container_of is actually the safer option as weird as that sounds. That said, we should do something to ensure that we get an assertion failure if it ever isn't the source of a phi. What you did above works. If we did container_of, we should add an assert loop somehow. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + } else { + return nir_before_instr(src->parent_instr); + } +} + static inline nir_cursor nir_before_cf_node(nir_cf_node *node) { diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c index dacf2d6c667..11c6693d302 100644 --- a/src/compiler/nir/nir_opt_if.c +++ b/src/compiler/nir/nir_opt_if.c @@ -369,6 +369,76 @@ opt_if_loop_terminator(nir_if *nif) return true; } +static void +replace_if_condition_use_with_const(nir_src *use, void *mem_ctx, + nir_cursor cursor, unsigned nir_boolean, </blockquote><div> </div><div>Mind making nir_boolean a uint32_t?</div><div> </div><div>With that (and regardless of what way we go with container_of above), this patch is</div><div> </div><div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + bool if_condition) +{ + /* Create const */ + nir_load_const_instr *load = nir_load_const_instr_create(mem_ctx, 1, 32); + load->value.u32[0] = nir_boolean; + nir_instr_insert(cursor, &load->instr); + + /* Rewrite use to use const */ + nir_src new_src = nir_src_for_ssa(&load->def); + + if (if_condition) + nir_if_rewrite_condition(use->parent_if, new_src); + else + nir_instr_rewrite_src(use->parent_instr, use, new_src); +} + +static bool +evaluate_if_condition(nir_if *nif, nir_cursor cursor, uint32_t *value) +{ + nir_block *use_block = nir_cursor_current_block(cursor); + if (nir_block_dominates(nir_if_first_then_block(nif), use_block)) { + *value = NIR_TRUE; + return true; + } else if (nir_block_dominates(nir_if_first_else_block(nif), use_block)) { + *value = NIR_FALSE; + return true; + } else { + return false; + } +} + +static bool +evaluate_condition_use(nir_if *nif, nir_src *use_src, void *mem_ctx, + bool is_if_condition) +{ + bool progress = false; + + uint32_t value; + nir_cursor cursor = nir_before_src(use_src, is_if_condition); + if (evaluate_if_condition(nif, cursor, &value)) { + replace_if_condition_use_with_const(use_src, mem_ctx, cursor, value, + is_if_condition); + progress = true; + } + + return progress; +} + +static bool +opt_if_evaluate_condition_use(nir_if *nif, void *mem_ctx) +{ + bool progress = false; + + /* Evaluate any uses of the if condition inside the if branches */ + assert(nif->condition.is_ssa); + nir_foreach_use_safe(use_src, nif->condition.ssa) { + progress |= evaluate_condition_use(nif, use_src, mem_ctx, false); + } + + nir_foreach_if_use_safe(use_src, nif->condition.ssa) { + if (use_src->parent_if != nif) + progress |= evaluate_condition_use(nif, use_src, mem_ctx, true); + } + + return progress; +} + static bool opt_if_cf_list(nir_builder *b, struct exec_list *cf_list) { @@ -402,6 +472,41 @@ opt_if_cf_list(nir_builder *b, struct exec_list *cf_list) return progress; } +/** + * These optimisations depend on nir_metadata_block_index and therefore must + * not do anything to cause the metadata to become invalid. + */ +static bool +opt_if_safe_cf_list(nir_builder *b, struct exec_list *cf_list, void *mem_ctx) +{ + bool progress = false; + foreach_list_typed(nir_cf_node, cf_node, node, cf_list) { + switch (cf_node->type) { + case nir_cf_node_block: + break; + + case nir_cf_node_if: { + nir_if *nif = nir_cf_node_as_if(cf_node); + progress |= opt_if_safe_cf_list(b, &nif->then_list, mem_ctx); + progress |= opt_if_safe_cf_list(b, &nif->else_list, mem_ctx); + progress |= opt_if_evaluate_condition_use(nif, mem_ctx); + break; + } + + case nir_cf_node_loop: { + nir_loop *loop = nir_cf_node_as_loop(cf_node); + progress |= opt_if_safe_cf_list(b, &loop->body, mem_ctx); + break; + } + + case nir_cf_node_function: + unreachable("Invalid cf type"); + } + } + + return progress; +} + bool nir_opt_if(nir_shader *shader) { @@ -414,6 +519,14 @@ nir_opt_if(nir_shader *shader) nir_builder b; nir_builder_init(&b, function->impl); + void *mem_ctx = ralloc_parent(function->impl); + + nir_metadata_require(function->impl, nir_metadata_block_index | + nir_metadata_dominance); + progress = opt_if_safe_cf_list(&b, &function->impl->body, mem_ctx); + nir_metadata_preserve(function->impl, nir_metadata_block_index | + nir_metadata_dominance); + if (opt_if_cf_list(&b, &function->impl->body)) { nir_metadata_preserve(function->impl, nir_metadata_none); -- 2.17.1 _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div></div>