[Mesa-dev] [PATCH] nir: evaluate if condition uses inside the if branches

[Timothy Arceri]

On 19/06/18 08:40, Ian Romanick wrote:
> On 06/17/2018 05:15 PM, 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 helped shaders are from Unreal Engine 4 besides one shader from
>> Dirt Showdown.
>>
>> shader-db results SKL:
>>
>> total instructions in shared programs: 13219725 -> 13219643 (<.01%)
>> instructions in affected programs: 28917 -> 28835 (-0.28%)
>> helped: 45
>> HURT: 0
>>
>> total cycles in shared programs: 529335971 -> 529334604 (<.01%)
>> cycles in affected programs: 216209 -> 214842 (-0.63%)
>> helped: 45
>> HURT: 4
>> ---
>>   src/compiler/nir/nir_opt_if.c | 84 +++++++++++++++++++++++++++++++----
>>   1 file changed, 76 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
>> index 863ca630fbd..2666c5da395 100644
>> --- a/src/compiler/nir/nir_opt_if.c
>> +++ b/src/compiler/nir/nir_opt_if.c
>> @@ -350,8 +350,53 @@ opt_if_loop_terminator(nir_if *nif)
>>      return true;
>>   }
>>   
>> +static void
>> +replace_if_condition_use_with_const(nir_src *use, nir_instr *instr,
>> +                                    unsigned nir_boolean, void *mem_ctx)
>> +{
>> +   /* 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_before(instr,  &load->instr);
>> +
>> +   /* Rewrite use to use const */
>> +   nir_src new_src = nir_src_for_ssa(&load->def);
>> +   nir_instr_rewrite_src(use->parent_instr, use, new_src);
>> +}
>> +
>>   static bool
>> -opt_if_cf_list(nir_builder *b, struct exec_list *cf_list)
>> +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 */
>> +   nir_foreach_use_safe(use_src, nif->condition.ssa) {
> 
> Does this iterate both the uses by regular instructions and the uses by
> other if-statements? 

Hmm ... good question. I believe we might need to add an additionl 
nir_foreach_if_use_safe() loop for that to work but it should be a 
simple addition.

> I was working on something similar (though using a
> different approach) because I came across some shaders that, after loop
> unrolling, had some neat stuff like
> 
>      if (ssa_111) {
>          ...
>          if (ssa_111) {
>              ...
>              if (ssa_111) {
>                  ...
>                  if (ssa_111) {
>                      ...
>                      if (ssa_111) {
>                          ...
>                          if (ssa_111) {
>                              ... >
>> +      nir_block *first_then = nir_if_first_then_block(nif);
>> +      if (use_src->parent_instr->block->index > first_then->index) {
>> +         nir_block *first_else = nir_if_first_else_block(nif);
>> +         if (use_src->parent_instr->block->index < first_else->index) {
>> +            nir_instr *instr = nir_block_first_instr(first_then);
>> +            replace_if_condition_use_with_const(use_src, instr,
>> +                                                NIR_TRUE, mem_ctx);
>> +
>> +            progress = true;
>> +         } else if (use_src->parent_instr->block->index <
>> +                    nir_if_last_else_block(nif)->index) {
>> +            nir_instr *instr = nir_block_first_instr(first_else);
>> +            replace_if_condition_use_with_const(use_src, instr,
>> +                                                NIR_FALSE, mem_ctx);
>> +
>> +            progress = true;
>> +         }
>> +      }
>> +   }
>> +
>> +   return progress;
>> +}
>> +
>> +static bool
>> +opt_if_cf_list(nir_builder *b, struct exec_list *cf_list, void *mem_ctx,
>> +               bool *md_altered)
>>   {
>>      bool progress = false;
>>      foreach_list_typed(nir_cf_node, cf_node, node, cf_list) {
>> @@ -361,17 +406,30 @@ opt_if_cf_list(nir_builder *b, struct exec_list *cf_list)
>>   
>>         case nir_cf_node_if: {
>>            nir_if *nif = nir_cf_node_as_if(cf_node);
>> -         progress |= opt_if_cf_list(b, &nif->then_list);
>> -         progress |= opt_if_cf_list(b, &nif->else_list);
>> -         progress |= opt_if_loop_terminator(nif);
>> -         progress |= opt_if_simplification(b, nif);
>> +         progress |= opt_if_cf_list(b, &nif->then_list, mem_ctx, md_altered);
>> +         progress |= opt_if_cf_list(b, &nif->else_list, mem_ctx, md_altered);
>> +
>> +         if (*md_altered == false)
>> +            progress |= opt_if_evaluate_condition_use(nif, mem_ctx);
>> +
>> +         bool lp_term_progress = opt_if_loop_terminator(nif);
>> +         bool if_simp_progress = opt_if_simplification(b, nif);
>> +         if (lp_term_progress || if_simp_progress) {
>> +            *md_altered = true;
>> +            progress = true;
>> +         }
>> +
>>            break;
>>         }
>>   
>>         case nir_cf_node_loop: {
>>            nir_loop *loop = nir_cf_node_as_loop(cf_node);
>> -         progress |= opt_if_cf_list(b, &loop->body);
>> -         progress |= opt_peel_loop_initial_if(loop);
>> +         progress |= opt_if_cf_list(b, &loop->body, mem_ctx, md_altered);
>> +         bool lp_peel_progress = opt_peel_loop_initial_if(loop);
>> +         if (lp_peel_progress) {
>> +            *md_altered = true;
>> +            progress = true;
>> +         }
>>            break;
>>         }
>>   
>> @@ -395,7 +453,17 @@ nir_opt_if(nir_shader *shader)
>>         nir_builder b;
>>         nir_builder_init(&b, function->impl);
>>   
>> -      if (opt_if_cf_list(&b, &function->impl->body)) {
>> +      nir_metadata_require(function->impl, nir_metadata_block_index);
>> +
>> +      /* opt_if_evaluate_condition_use() depends on nir_metadata_block_index
>> +       * if one of the other opts has altered the block in some way we need to
>> +       * make sure we skip this opt.
>> +       */
>> +      bool index_metadata_altered = false;
>> +
>> +      void *mem_ctx = ralloc_parent(function->impl);
>> +      if (opt_if_cf_list(&b, &function->impl->body, mem_ctx,
>> +                         &index_metadata_altered)) {
>>            nir_metadata_preserve(function->impl, nir_metadata_none);
>>   
>>            /* If that made progress, we're no longer really in SSA form.  We
>>
>