[Mesa-dev] [PATCH 1/4] nir: add if opt opt_if_loop_last_continue()

[Samuel Pitoiset]

This introduces crashes for

dEQP-VK.spirv_assembly.instruction.graphics.selection_block_order.out_of_order_frag
dEQP-VK.spirv_assembly.instruction.graphics.selection_block_order.out_of_order_geom
dEQP-VK.spirv_assembly.instruction.graphics.selection_block_order.out_of_order_tessc
dEQP-VK.spirv_assembly.instruction.graphics.selection_block_order.out_of_order_tesse
dEQP-VK.spirv_assembly.instruction.graphics.selection_block_order.out_of_order_vert

Test case 
'dEQP-VK.spirv_assembly.instruction.graphics.selection_block_order.out_of_order_frag'..
deqp-vk: nir/nir_control_flow.c:553: stitch_blocks: Assertion 
`exec_list_is_empty(&after->instr_list)' failed.
Aborted (core dumped)

Did you run CTS?

On 11/28/18 4:25 AM, Timothy Arceri wrote:
> From: Danylo Piliaiev <danylo.piliaiev at gmail.com>
> 
> Removing the last continue can allow more loops to unroll. Also
> inserting code into the if branch can allow the various if opts
> to progress further.
> 
> The insertion of some loops into the if branch also reduces VGPR
> use in some shaders.
> 
> vkpipeline-db results (VEGA):
> 
> Totals from affected shaders:
> SGPRS: 6552 -> 6576 (0.37 %)
> VGPRS: 6544 -> 6532 (-0.18 %)
> Spilled SGPRs: 0 -> 0 (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: 481952 -> 478032 (-0.81 %) bytes
> LDS: 13 -> 13 (0.00 %) blocks
> Max Waves: 241 -> 242 (0.41 %)
> Wait states: 0 -> 0 (0.00 %)
> 
> Shader-db results radeonsi (VEGA):
> 
> Totals from affected shaders:
> SGPRS: 168 -> 168 (0.00 %)
> VGPRS: 144 -> 140 (-2.78 %)
> Spilled SGPRs: 157 -> 157 (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: 8524 -> 8488 (-0.42 %) bytes
> LDS: 0 -> 0 (0.00 %) blocks
> Max Waves: 7 -> 7 (0.00 %)
> Wait states: 0 -> 0 (0.00 %)
> 
> v2: (Timothy Arceri):
> - allow for continues in either branch
> - move any trailing loops inside the if as well as blocks.
> - leave nir_opt_trivial_continues() to actually remove the
>    continue.
> 
> Signed-off-by: Timothy Arceri <tarceri at itsqueeze.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=32211
> ---
>   src/compiler/nir/nir_opt_if.c | 95 +++++++++++++++++++++++++++++++++++
>   1 file changed, 95 insertions(+)
> 
> diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
> index dd488b1787..4a9dffb782 100644
> --- a/src/compiler/nir/nir_opt_if.c
> +++ b/src/compiler/nir/nir_opt_if.c
> @@ -263,6 +263,100 @@ rewrite_phi_predecessor_blocks(nir_if *nif,
>      }
>   }
>   
> +static bool
> +nir_block_ends_in_continue(nir_block *block)
> +{
> +   if (exec_list_is_empty(&block->instr_list))
> +      return false;
> +
> +   nir_instr *instr = nir_block_last_instr(block);
> +   return instr->type == nir_instr_type_jump &&
> +      nir_instr_as_jump(instr)->type == nir_jump_continue;
> +}
> +
> +/**
> + * This optimization turns:
> + *
> + *     loop {
> + *        ...
> + *        if (cond) {
> + *           do_work_1();
> + *           continue;
> + *        } else {
> + *        }
> + *        do_work_2();
> + *     }
> + *
> + * into:
> + *
> + *     loop {
> + *        ...
> + *        if (cond) {
> + *           do_work_1();
> + *           continue;
> + *        } else {
> + *           do_work_2();
> + *        }
> + *     }
> + *
> + * The continue should then be removed by nir_opt_trivial_continues() and the
> + * loop can potentially be unrolled.
> + *
> + * Note: do_work_2() is only ever blocks and nested loops. We could also nest
> + * other if-statments in the branch which would allow further continues to
> + * be removed. However in practice this can result in increased register
> + * pressure.
> + */
> +static bool
> +opt_if_loop_last_continue(nir_loop *loop)
> +{
> +   /* Get the last if-stament in the loop */
> +   nir_block *last_block = nir_loop_last_block(loop);
> +   nir_cf_node *if_node = nir_cf_node_prev(&last_block->cf_node);
> +   while (if_node) {
> +      if (if_node->type == nir_cf_node_if)
> +         break;
> +
> +      if_node = nir_cf_node_prev(if_node);
> +   }
> +
> +   if (!if_node || if_node->type != nir_cf_node_if)
> +      return false;
> +
> +   nir_if *nif = nir_cf_node_as_if(if_node);
> +   nir_block *then_block = nir_if_last_then_block(nif);
> +   nir_block *else_block = nir_if_last_else_block(nif);
> +
> +   bool then_ends_in_continue = nir_block_ends_in_continue(then_block);
> +   bool else_ends_in_continue = nir_block_ends_in_continue(else_block);
> +
> +   /* If both branches end in a continue do nothing, this should be handled
> +    * by nir_opt_dead_cf().
> +    */
> +   if (then_ends_in_continue && else_ends_in_continue)
> +      return false;
> +
> +   if (!then_ends_in_continue && !else_ends_in_continue)
> +      return false;
> +
> +   /* Move the last block of the loop inside the last if-statement */
> +   nir_cf_list tmp;
> +   nir_cf_extract(&tmp, nir_after_cf_node(if_node),
> +                        nir_after_block(last_block));
> +   if (then_ends_in_continue) {
> +      nir_cf_reinsert(&tmp, nir_after_cf_list(&nif->else_list));
> +   } else {
> +      nir_cf_reinsert(&tmp, nir_after_cf_list(&nif->then_list));
> +   }
> +
> +   /* In order to avoid running nir_lower_regs_to_ssa_impl() every time an if
> +    * opt makes progress we leave nir_opt_trivial_continues() to remove the
> +    * continue now that the end of the loop has been simplified.
> +    */
> +
> +   return true;
> +}
> +
>   /**
>    * This optimization turns:
>    *
> @@ -700,6 +794,7 @@ opt_if_cf_list(nir_builder *b, struct exec_list *cf_list)
>            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_loop_last_continue(loop);
>            break;
>         }
>   
>