On Wed, Aug 29, 2018 at 7:58 PM Ian Romanick <i...@freedesktop.org> 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); > > + 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. If this pass > also used the shader as the memory context, most places could eliminate > the mem_ctx parameter and just use b->shader. It probably doesn't make > that much difference either way. > Thanks for noticing that! Yes, we should just be using b->shader everywhere instead. There's no point in passing around a mem_ctx just so we can allocate one instruction when we're already passing around a builder. --Jason
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev