On Mon, Jul 23, 2018 at 3:03 AM Timothy Arceri <tarc...@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.
>
> 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_opt_if.c | 124 ++++++++++++++++++++++++++++++++++
> 1 file changed, 124 insertions(+)
>
> diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
> index b3d0bf1decb..b3d5046a76e 100644
> --- a/src/compiler/nir/nir_opt_if.c
> +++ b/src/compiler/nir/nir_opt_if.c
> @@ -369,6 +369,87 @@ opt_if_loop_terminator(nir_if *nif)
> return true;
> }
>
> +static void
> +replace_if_condition_use_with_const(nir_src *use, unsigned nir_boolean,
> + void *mem_ctx, 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;
> +
> + if (if_condition) {
> + nir_instr_insert_before_cf(&use->parent_if->cf_node, &load->instr);
>
If it was me, I'd probably use the builder but I think it's a wash in this
case.
> + } else if (use->parent_instr->type == nir_instr_type_phi) {
> + nir_phi_instr *cond_phi = nir_instr_as_phi(use->parent_instr);
> +
> + bool UNUSED found = false;
> + nir_foreach_phi_src(phi_src, cond_phi) {
> + if (phi_src->src.ssa == use->ssa) {
>
You could also just use some sort of container_of macro to cast from the
src to a phi_src. It's a bit sneaky so maybe not a good idea for the tiny
bit of perf.
> + nir_instr_insert_before_block(phi_src->pred, &load->instr);
>
after_block_before_jump would work just as well and would put the
load_const closer to its use.
> + found = true;
> + break;
> + }
> + }
> + assert(found);
> + } else {
> + nir_instr_insert_before(use->parent_instr, &load->instr);
> + }
> +
> + /* Rewrite use to use const */
> + nir_src new_src = nir_src_for_ssa(&load->def);
>
Is there a good reason for the temporary variable?
> +
> + if (if_condition)
> + nir_if_rewrite_condition(use->parent_if, new_src);
> + else
> + nir_instr_rewrite_src(use->parent_instr, use, new_src);
> +}
>
Ok, enough nitpicking. None of the above things are actually problems.
> +
> +static bool
> +evaluate_condition_use(nir_if *nif, nir_src *use_src, void *mem_ctx,
> + bool if_condition)
> +{
> + bool progress = false;
> +
> + nir_block *use_block;
> + if (if_condition) {
> + use_block =
> +
> nir_cf_node_as_block(nir_cf_node_prev(&use_src->parent_if->cf_node));
> + } else {
> + use_block = use_src->parent_instr->block;
>
Not true for phis!
> + }
> +
> + if (nir_block_dominates(nir_if_first_then_block(nif), use_block)) {
> + replace_if_condition_use_with_const(use_src, NIR_TRUE, mem_ctx,
> + if_condition);
> + progress = true;
> + } else if (nir_block_dominates(nir_if_first_else_block(nif),
> use_block)) {
> + replace_if_condition_use_with_const(use_src, NIR_FALSE, mem_ctx,
> + if_condition);
> + progress = true;
> + }
> +
> + return progress;
> +}
>
I think things would be more straightforward (and correct!) if you merged
the above two functions and restructured them a bit as follows:
static bool
try_rewrite_if_use(nir_builder *b, nir_if *nif, nir_src *src, bool
if_condition)
{
if (if_condition) {
b->cursor = nir_before_cf_node(&nif->cf_node);
} else if (src->parent_instr->type == nir_instr_type_phi) {
// Set the cursor and use_block to the predecessor block
} else {
b->cursor = nir_before_instr(src->parent_instr);
}
nir_block *use_block = nir_cursor_current_block(b->cursor);
nir_ssa_def *const_val;
if (nir_block_dominates(nir_if_first_then_block(nif), use_block))
const_value = nir_imm_int(b, NIR_TRUE);
else if (nir_block_dominates(nir_if_first_else_block(nif), use_block)
const_value = nir_imm_int(b, NIR_FALSE);
else
return false;
// Rewrite the source
return true;
}
Other than the phi issue I pointed out above (which would be fixed by that
refactor), everything looks good to me.
--Jason
> +
> +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 +483,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 +530,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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
