[Mesa-dev] [PATCH 1/3] nir/peephole_select: Rename are_all_move_to_phi and use a switch

[Connor Abbott]

On Mon, Mar 16, 2015 at 9:23 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> ---
>  src/glsl/nir/nir_opt_peephole_select.c | 54 +++++++++++++++++++---------------
>  1 file changed, 30 insertions(+), 24 deletions(-)
>
> diff --git a/src/glsl/nir/nir_opt_peephole_select.c b/src/glsl/nir/nir_opt_peephole_select.c
> index ab08f28..8064f40 100644
> --- a/src/glsl/nir/nir_opt_peephole_select.c
> +++ b/src/glsl/nir/nir_opt_peephole_select.c
> @@ -52,36 +52,41 @@ struct peephole_select_state {
>  };
>
>  static bool
> -are_all_move_to_phi(nir_block *block)
> +block_check_for_allowed_instrs(nir_block *block)
>  {
>     nir_foreach_instr(block, instr) {
> -      if (instr->type != nir_instr_type_alu)
> -         return false;
> +      switch (instr->type) {
> +      case nir_instr_type_alu: {
> +         /* It must be a move operation */
> +         nir_alu_instr *mov = nir_instr_as_alu(instr);
> +         if (mov->op != nir_op_fmov && mov->op != nir_op_imov)
> +            return false;
>
> -      /* It must be a move operation */
> -      nir_alu_instr *mov = nir_instr_as_alu(instr);
> -      if (mov->op != nir_op_fmov && mov->op != nir_op_imov)
> -         return false;
> +         /* Can't handle saturate */
> +         if (mov->dest.saturate)
> +            return false;
>
> -      /* Can't handle saturate */
> -      if (mov->dest.saturate)
> -         return false;
> +         /* It must be SSA */
> +         if (!mov->dest.dest.is_ssa)
> +            return false;
>
> -      /* It must be SSA */
> -      if (!mov->dest.dest.is_ssa)
> -         return false;
> +         /* It cannot have any if-uses */
> +         if (mov->dest.dest.ssa.if_uses->entries != 0)
> +            return false;
>
> -      /* It cannot have any if-uses */
> -      if (mov->dest.dest.ssa.if_uses->entries != 0)
> -         return false;
> +         /* The only uses of this definition must be phi's in the successor */
> +         struct set_entry *entry;
> +         set_foreach(mov->dest.dest.ssa.uses, entry) {
> +            const nir_instr *dest_instr = entry->key;
> +            if (dest_instr->type != nir_instr_type_phi ||
> +                dest_instr->block != block->successors[0])
> +               return false;
> +         }
> +         break;
> +      }
>
> -      /* The only uses of this definition must be phi's in the successor */
> -      struct set_entry *entry;
> -      set_foreach(mov->dest.dest.ssa.uses, entry) {
> -         const nir_instr *dest_instr = entry->key;
> -         if (dest_instr->type != nir_instr_type_phi ||
> -             dest_instr->block != block->successors[0])
> -            return false;
> +      default:
> +         return false;
>        }
>     }
>
> @@ -120,7 +125,8 @@ nir_opt_peephole_select_block(nir_block *block, void *void_state)
>     nir_block *else_block = nir_cf_node_as_block(else_node);
>
>     /* ... and those blocks must only contain move-to-phi. */

You should change this comment in patch 3 before it becomes stale.
Otherwise, the series is

Reviewed-by: Connor Abbott <cwabbott0 at gmail.com>

So a good improvement, but not quite parity with GLSL IR yet. Any
ideas what other silly things like this are holding us back from
finally doing better?

> -   if (!are_all_move_to_phi(then_block) || !are_all_move_to_phi(else_block))
> +   if (!block_check_for_allowed_instrs(then_block) ||
> +       !block_check_for_allowed_instrs(else_block))
>        return true;
>
>     /* At this point, we know that the previous CFG node is an if-then
> --
> 2.3.2
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev