On Wed, Aug 29, 2018 at 9:45 PM Timothy Arceri <tarc...@itsqueeze.com> wrote:
> 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); > The one actual use of mem_ctx is right here. > >> + 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. > 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. --Jason
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev