[Mesa-dev] [PATCH v4 1/7] nir: evaluate if condition uses inside the if branches
Jason Ekstrand
jason at jlekstrand.net
Thu Aug 30 02:41:09 UTC 2018
On Wed, Aug 29, 2018 at 7:58 PM Ian Romanick <idr at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180829/3580c1d2/attachment.html>
More information about the mesa-dev
mailing list