On Mon, Mar 16, 2015 at 6:52 PM, Connor Abbott <cwabbo...@gmail.com> wrote:
> On Mon, Mar 16, 2015 at 9:23 PM, Jason Ekstrand <ja...@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 <cwabbo...@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@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
