[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 10:08 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> 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.

That's fine by me.

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