On Fri, Aug 17, 2018 at 1:13 PM Jason Ekstrand <ja...@jlekstrand.net> wrote:
> 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.
>
Scratch that! It doesn't work at all with the second patch. With the
second patch, I think I understand and I think the structure you have works
well. I would, however, suggest we refactor some things into a couple of
helpers:
First, I just sent a patch which pulls two different implementations of
ends_in_jump into nir.h and unifies them.
/* In nir.h */
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));
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_src *phi_src = NULL;
phi_src = container_of(src, phi_src, src);
return nir_after_block_before_jump(phi_src->pred);
} else {
return nir_before_instr(src->parent_instr);
}
}
/* in nir_opt_if.c */
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;
}
}
Then you can use those two instead of open-coding them all over the place.
The first would prevent the phi bug mentioned above from propagating and
the second would just save some code and make some things easier.
--Jason
> --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
