On Wed, Aug 29, 2018 at 9:45 PM Timothy Arceri <tarc...@itsqueeze.com
<mailto:tarc...@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.
