[Mesa-dev] [PATCH 2/2] nir: merge some basic consecutive ifs

[Ian Romanick]

On 11/26/2018 09:31 PM, Timothy Arceri wrote:
> After trying multiple times to merge if-statements with phis
> between them I've come to the conclusion that it cannot be done
> without regressions. The problem is for some shaders we end up
> with a whole bunch of phis for the merged ifs resulting in
> increased register pressure.
> 
> So this patch just merges ifs that have no phis between them.
> This seems to be consistent with what LLVM does so for radeonsi
> we only see a change (although its a large change) in a single
> shader.
> 
> Shader-db results i965 (SKL):
> 
> total instructions in shared programs: 13098176 -> 13098152 (<.01%)
> instructions in affected programs: 1326 -> 1302 (-1.81%)
> helped: 4
> HURT: 0
> 
> total cycles in shared programs: 332032989 -> 332037583 (<.01%)
> cycles in affected programs: 60665 -> 65259 (7.57%)
> helped: 0
> HURT: 4
> 
> The cycles estimates reported by shader-db for i965 seem inaccurate
> as the only difference in the final code is the removal of the
> redundent condition evaluations and jumps.

The scheduling doesn't even change?  Whenever I've encountered things
like this, the scheduler has been to blame.  It seems surprising that
only 4 shaders are affected.  Are these all different instances of the
same shader? :)

> Also the biggest code reduction (~7%) for radeonsi was in a tomb
> raider tressfx shader but for some reason this does not get merged
> for i965.
> 
> Shader-db results radeonsi (VEGA):
> 
> Totals from affected shaders:
> SGPRS: 232 -> 232 (0.00 %)
> VGPRS: 164 -> 164 (0.00 %)
> Spilled SGPRs: 59 -> 59 (0.00 %)
> Spilled VGPRs: 0 -> 0 (0.00 %)
> Private memory VGPRs: 0 -> 0 (0.00 %)
> Scratch size: 0 -> 0 (0.00 %) dwords per thread
> Code Size: 14584 -> 13520 (-7.30 %) bytes
> LDS: 0 -> 0 (0.00 %) blocks
> Max Waves: 13 -> 13 (0.00 %)
> Wait states: 0 -> 0 (0.00 %)
> ---
>  src/compiler/nir/nir_opt_if.c | 93 +++++++++++++++++++++++++++++++++++
>  1 file changed, 93 insertions(+)
> 
> diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
> index 62566eb403..dd488b1787 100644
> --- a/src/compiler/nir/nir_opt_if.c
> +++ b/src/compiler/nir/nir_opt_if.c
> @@ -585,6 +585,98 @@ opt_if_evaluate_condition_use(nir_builder *b, nir_if *nif)
>     return progress;
>  }
>  
> +static void
> +simple_merge_if(nir_if *dest_if, nir_if *src_if, bool dest_if_then,
> +                bool src_if_then)
> +{
> +   /* Now merge the if branch */
> +   nir_block *dest_blk = dest_if_then ? nir_if_last_then_block(dest_if)
> +                                      : nir_if_last_else_block(dest_if);
> +
> +   struct exec_list *list = src_if_then ? &src_if->then_list
> +                                        : &src_if->else_list;
> +
> +   nir_cf_list if_cf_list;
> +   nir_cf_extract(&if_cf_list, nir_before_cf_list(list),
> +                  nir_after_cf_list(list));
> +   nir_cf_reinsert(&if_cf_list, nir_after_block(dest_blk));
> +}
> +
> +static bool
> +opt_if_merge(nir_if *nif)
> +{
> +   bool progress = false;
> +
> +   nir_block *next_blk = nir_cf_node_cf_tree_next(&nif->cf_node);
> +   if (next_blk && nif->condition.is_ssa) {
> +      nir_if *next_if = nir_block_get_following_if(next_blk);
> +      if (next_if && next_if->condition.is_ssa) {
> +
> +         /* Here we merge two consecutive ifs that have the same
> +          * condition e.g:
> +          *
> +          *   if ssa_12 {
> +          *      ...
> +          *   } else {
> +          *      ...
> +          *   }
> +          *   if ssa_12 {
> +          *      ...
> +          *   } else {
> +          *      ...
> +          *   }
> +          *
> +          * Note: This only merges if-statements when the block between them
> +          * is empty. The reason we don't try to merge ifs that just have phis
> +          * between them is because this can results in increased register
> +          * pressure. For example when merging if ladders created by indirect
> +          * indexing.
> +          */
> +         if (nif->condition.ssa == next_if->condition.ssa &&
> +             exec_list_is_empty(&next_blk->instr_list)) {
> +
> +            simple_merge_if(nif, next_if, true, true);
> +            simple_merge_if(nif, next_if, false, false);
> +
> +            nir_block *new_then_block = nir_if_last_then_block(nif);
> +            nir_block *new_else_block = nir_if_last_else_block(nif);
> +
> +            nir_block *old_then_block = nir_if_last_then_block(next_if);
> +            nir_block *old_else_block = nir_if_last_else_block(next_if);
> +
> +            /* Rewrite the predecessor block for any phis following the second
> +             * if-statement.
> +             */
> +            rewrite_phi_predecessor_blocks(next_if, old_then_block,
> +                                           old_else_block,
> +                                           new_then_block,
> +                                           new_else_block);
> +
> +            /* Move phis after merged if to avoid them being deleted when we
> +             * remove the merged if-statement.
> +             */
> +            nir_block *after_next_if_block =
> +               nir_cf_node_as_block(nir_cf_node_next(&next_if->cf_node));
> +
> +            nir_foreach_instr_safe(instr, after_next_if_block) {
> +               if (instr->type != nir_instr_type_phi)
> +                  break;
> +
> +               exec_node_remove(&instr->node);
> +               exec_list_push_tail(&next_blk->instr_list, &instr->node);
> +               instr->block = next_blk;
> +            }
> +
> +            nir_cf_node_remove(&next_if->cf_node);
> +
> +            progress = true;
> +         }
> +      }
> +   }
> +
> +   return progress;
> +}
> +
>  static bool
>  opt_if_cf_list(nir_builder *b, struct exec_list *cf_list)
>  {
> @@ -599,6 +691,7 @@ opt_if_cf_list(nir_builder *b, struct exec_list *cf_list)
>           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_merge(nif);
>           progress |= opt_if_simplification(b, nif);
>           break;
>        }
>