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

[Jason Ekstrand]

On Mon, Mar 16, 2015 at 6:52 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
> 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

I changed it in patch 1 because that's where we did the rename.  I
hope that's ok.

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

Thanks!

> 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?

There are a couple issues with constant-combine (Matt has a patch),
I've got a patch to fix up types on some things which will hopefully
help after matt's patch.  Then we have way too much mov-to-flag.  I'm
going to play with that last one a bit tonight.
--Jason

>> -   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