[Mesa-dev] [PATCH v4 1/7] nir: evaluate if condition uses inside the if branches
Jason Ekstrand
jason at jlekstrand.net
Tue Aug 28 15:30:16 UTC 2018
On Mon, Aug 27, 2018 at 4:09 AM Timothy Arceri <tarceri at itsqueeze.com>
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);
> + }
> + }
>
This works. Some sort of container_of would be more efficient. I guess
that wouldn't work if the source was a register indirect on a phi src but I
don't think that's valid.
> +
> + unreachable("failed to find phi src");
>
If the source isn't a phi src (and container_of would fail) hitting an
unreachable is really nasty. I don't think we will hit this case but I
also think that if we do, container_of is actually the safer option as
weird as that sounds. That said, we should do something to ensure that we
get an assertion failure if it ever isn't the source of a phi. What you
did above works. If we did container_of, we should add an assert loop
somehow.
> + } 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,
>
Mind making nir_boolean a uint32_t?
With that (and regardless of what way we go with container_of above), this
patch is
Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>
> + 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);
> +
> + nir_metadata_require(function->impl, nir_metadata_block_index |
> + nir_metadata_dominance);
> + progress = opt_if_safe_cf_list(&b, &function->impl->body, mem_ctx);
> + nir_metadata_preserve(function->impl, nir_metadata_block_index |
> + nir_metadata_dominance);
> +
> if (opt_if_cf_list(&b, &function->impl->body)) {
> nir_metadata_preserve(function->impl, nir_metadata_none);
>
> --
> 2.17.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180828/3742851b/attachment.html>
More information about the mesa-dev
mailing list