<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Wed, Aug 29, 2018 at 9:45 PM Timothy Arceri <<a href="mailto:tarceri@itsqueeze.com">tarceri@itsqueeze.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 30/08/18 10:57, Ian Romanick wrote: > On 08/27/2018 02:08 AM, Timothy Arceri wrote: >> 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); >> + } >> + } >> + >> + unreachable("failed to find phi src"); >> + } 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, >> + bool if_condition) >> +{ >> + /* Create const */ >> + nir_load_const_instr *load = nir_load_const_instr_create(mem_ctx, 1, 32); </blockquote><div> </div><div>The one actual use of mem_ctx is right here. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> >> + 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); > > Is this the right thing to use for the memory context? It looks like > most other places use the shader as the memory context. I had a quick look and to me is seems most places use impl. Maybe you could provide an example? Although I plan on pushing the series as it's been on the list for a long time now and I've finally collected Jason's rb on each patch. As you say its not much difference either way. </blockquote><div> </div><div>You do a lot of passing of mem_ctx around just so that you can use it in exactly one place. That one place could take a builder instead of a mem_ctx and a cursor.</div><div> </div><div>--Jason </div></div></div>