[Mesa-dev] [PATCH 07/10] nir: add loop unroll support for wrapper loops

[Timothy Arceri]

On 22/07/18 19:50, Jason Ekstrand wrote:
> On Tue, Jul 10, 2018 at 11:49 PM Timothy Arceri <tarceri at itsqueeze.com 
> <mailto:tarceri at itsqueeze.com>> wrote:
> 
>     This adds support for unrolling the classic
> 
>          do {
>              // ...
>          } while (false)
> 
>     that is used to wrap multi-line macros. GLSL IR also wraps switch
>     statements in a loop like this.
> 
>     shader-db results IVB:
> 
>     total loops in shared programs: 2515 -> 2512 (-0.12%)
>     loops in affected programs: 33 -> 30 (-9.09%)
>     helped: 3
>     HURT: 0
>     ---
>       src/compiler/nir/nir_opt_loop_unroll.c | 63 ++++++++++++++++++++++++++
>       1 file changed, 63 insertions(+)
> 
>     diff --git a/src/compiler/nir/nir_opt_loop_unroll.c
>     b/src/compiler/nir/nir_opt_loop_unroll.c
>     index 38901470f96..f715fc8ebda 100644
>     --- a/src/compiler/nir/nir_opt_loop_unroll.c
>     +++ b/src/compiler/nir/nir_opt_loop_unroll.c
>     @@ -516,6 +516,69 @@ process_loops(nir_shader *sh, nir_cf_node
>     *cf_node, bool *has_nested_loop)
>           */
>          if (!progress) {
> 
>     +      /* Check for the the classic
>     +       *
>     +       *    do {
>     +       *        // ...
>     +       *    } while (false)
>     +       *
>     +       * that is used to wrap multi-line macros. GLSL IR also wraps
>     switch
>     +       * statements in a loop like this.
>     +       */
>     +      if (loop->info->limiting_terminator == NULL &&
>     +          list_empty(&loop->info->loop_terminator_list) &&
>     +          !loop->info->complex_loop) {
>     +
> 
> 
> Mind putting all the code below in a "wrapper_unroll" or 
> "trivial_unroll" helper?
> 
>     +         nir_block *blk_after_loop =
>     +           
>     nir_cursor_current_block(nir_after_cf_node(&loop->cf_node));
>     +
>     +         /* There may still be some single src phis following the
>     loop that
>     +          * have not yet been cleaned up by another pass. Tidy
>     those up before
>     +          * unrolling the loop.
>     +          */
>     +         nir_foreach_instr_safe(instr, blk_after_loop) {
>     +            if (instr->type != nir_instr_type_phi)
>     +               break;
>     +
>     +            nir_phi_instr *phi = nir_instr_as_phi(instr);
>     +            assert(exec_list_length(&phi->srcs) == 1);
>     +
>     +            nir_phi_src *phi_src =
>     +               exec_node_data(nir_phi_src,
>     +                              exec_list_get_head(&phi->srcs),
>     +                              node);
>     +
>     +            nir_ssa_def_rewrite_uses(&phi->dest.ssa, phi_src->src);
>     +            nir_instr_remove(instr);
>     +
>     +            progress = true;
>     +         }
>     +
>     +         nir_block *last_loop_blk = nir_loop_last_block(loop);
>     +         if (nir_block_ends_in_break(last_loop_blk)) {
> 
> 
> This confuses me.  Above, you assert that all phis have exactly one 
> source and here you have an if (nir_block_ends_in_break(last_block)); 
> which one is it?  Either our loop terminator check above guarantees that 
> we're in the "do { } while (true)" case where we can assume a single 
> exit at the end of the loop or it doesn't.  Which one is it?

Actually ignore my previous reply this was added because there may still 
be dead instructions following the break that are yet to be removed.