[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:56:33 UTC 2018
On Wed, Aug 29, 2018 at 9:45 PM Timothy Arceri <tarceri at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180829/fee4edff/attachment-0001.html>
More information about the mesa-dev
mailing list