[Mesa-dev] [RFC] nir: Use an instruction for the condition on if statements
Connor Abbott
cwabbott0 at gmail.com
Sat Feb 28 10:33:17 PST 2015
On Fri, Feb 27, 2015 at 8:13 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> Previously, the nir_if control-flow node had a source built straight into
> it that was the if condition. This has been the source of a lot of
> edge-case headaches due to, in particular, the two different use sets that
> we were carrying around. This patch changes it to have a special jump
> instruction that gets placed at the end of the block in front of the if.
> This way we no longer have to keep special-casing the source and can treate
> it like any other use in an instruction.
>
> Cc: Connor Abbott <cwabbott0 at gmail.com>
So at first my gut reaction is that this was a bad idea -- we would be
splitting off part of the if, which is part of the control flow, into
a separate instruction when all the other control flow stuff is part
of the tree. But we already use instructions to represent breaks and
continues, and clearly the ugliness that is if_uses dying is good
enough that I can't possibly argue against this. Is there a reason you
marked this as RFC? I remember that you stopped working on this a
while ago because of some problem with the control-flow modification
code in nir.c -- did you ever figure that out?
>
> ---
> src/glsl/nir/glsl_to_nir.cpp | 17 ++++--
> src/glsl/nir/nir.c | 70 +++++++----------------
> src/glsl/nir/nir.h | 11 ++--
> src/glsl/nir/nir_from_ssa.c | 18 ------
> src/glsl/nir/nir_live_variables.c | 4 --
> src/glsl/nir/nir_lower_to_source_mods.c | 6 +-
> src/glsl/nir/nir_opt_copy_propagate.c | 46 +++-------------
> src/glsl/nir/nir_opt_dce.c | 7 ---
> src/glsl/nir/nir_opt_gcm.c | 12 ----
> src/glsl/nir/nir_opt_global_to_local.c | 11 ----
> src/glsl/nir/nir_opt_peephole_select.c | 21 ++++---
> src/glsl/nir/nir_print.c | 9 ++-
> src/glsl/nir/nir_to_ssa.c | 16 +-----
> src/glsl/nir/nir_validate.c | 95 ++++++++++++--------------------
> src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 15 +++--
> 15 files changed, 111 insertions(+), 247 deletions(-)
>
> diff --git a/src/glsl/nir/glsl_to_nir.cpp b/src/glsl/nir/glsl_to_nir.cpp
> index adef19c..e84bf1c 100644
> --- a/src/glsl/nir/glsl_to_nir.cpp
> +++ b/src/glsl/nir/glsl_to_nir.cpp
> @@ -536,12 +536,13 @@ nir_visitor::visit(ir_loop *ir)
> void
> nir_visitor::visit(ir_if *ir)
> {
> - nir_src condition = evaluate_rvalue(ir->condition);
> + nir_jump_instr *if_instr = nir_jump_instr_create(this->shader, nir_jump_if);
> + if_instr->if_src = evaluate_rvalue(ir->condition);
> + nir_instr_insert_after_cf_list(this->cf_node_list, &if_instr->instr);
>
> exec_list *old_list = this->cf_node_list;
>
> nir_if *if_stmt = nir_if_create(this->shader);
> - if_stmt->condition = condition;
> nir_cf_node_insert_end(old_list, &if_stmt->cf_node);
>
> this->cf_node_list = &if_stmt->then_list;
> @@ -704,9 +705,13 @@ nir_visitor::visit(ir_assignment *ir)
>
>
> if (ir->condition) {
> + nir_jump_instr *if_instr = nir_jump_instr_create(this->shader, nir_jump_if);
> + if_instr->if_src = evaluate_rvalue(ir->condition);
> + nir_instr_insert_after_cf_list(this->cf_node_list, &if_instr->instr);
> +
> nir_if *if_stmt = nir_if_create(this->shader);
> - if_stmt->condition = evaluate_rvalue(ir->condition);
> nir_cf_node_insert_end(this->cf_node_list, &if_stmt->cf_node);
> +
> nir_instr_insert_after_cf_list(&if_stmt->then_list, ©->instr);
> } else {
> nir_instr_insert_after_cf_list(this->cf_node_list, ©->instr);
> @@ -779,9 +784,13 @@ nir_visitor::visit(ir_assignment *ir)
> store->src[0] = src;
>
> if (ir->condition) {
> + nir_jump_instr *if_instr = nir_jump_instr_create(this->shader, nir_jump_if);
> + if_instr->if_src = evaluate_rvalue(ir->condition);
> + nir_instr_insert_after_cf_list(this->cf_node_list, &if_instr->instr);
> +
> nir_if *if_stmt = nir_if_create(this->shader);
> - if_stmt->condition = evaluate_rvalue(ir->condition);
> nir_cf_node_insert_end(this->cf_node_list, &if_stmt->cf_node);
> +
> nir_instr_insert_after_cf_list(&if_stmt->then_list, &store->instr);
> } else {
> nir_instr_insert_after_cf_list(this->cf_node_list, &store->instr);
> diff --git a/src/glsl/nir/nir.c b/src/glsl/nir/nir.c
> index ab57fd4..bd668ee 100644
> --- a/src/glsl/nir/nir.c
> +++ b/src/glsl/nir/nir.c
> @@ -68,8 +68,6 @@ reg_create(void *mem_ctx, struct exec_list *list)
> _mesa_key_pointer_equal);
> reg->defs = _mesa_set_create(mem_ctx, _mesa_hash_pointer,
> _mesa_key_pointer_equal);
> - reg->if_uses = _mesa_set_create(mem_ctx, _mesa_hash_pointer,
> - _mesa_key_pointer_equal);
>
> reg->num_components = 0;
> reg->num_array_elems = 0;
> @@ -317,7 +315,6 @@ nir_if_create(void *mem_ctx)
> nir_if *if_stmt = ralloc(mem_ctx, nir_if);
>
> cf_init(&if_stmt->cf_node, nir_cf_node_if);
> - src_init(&if_stmt->condition);
>
> nir_block *then = nir_block_create(mem_ctx);
> exec_list_make_empty(&if_stmt->then_list);
> @@ -409,6 +406,7 @@ nir_jump_instr_create(void *mem_ctx, nir_jump_type type)
> nir_jump_instr *instr = ralloc(mem_ctx, nir_jump_instr);
> instr_init(&instr->instr, nir_instr_type_jump);
> instr->type = type;
> + src_init(&instr->if_src);
> return instr;
> }
>
> @@ -860,8 +858,6 @@ handle_jump(nir_block *block)
> nir_instr *instr = nir_block_last_instr(block);
> nir_jump_instr *jump_instr = nir_instr_as_jump(instr);
>
> - unlink_block_successors(block);
> -
> nir_function_impl *impl = nir_cf_node_get_function(&block->cf_node);
> nir_metadata_preserve(impl, nir_metadata_none);
>
> @@ -869,6 +865,8 @@ handle_jump(nir_block *block)
> jump_instr->type == nir_jump_continue) {
> nir_loop *loop = nearest_loop(&block->cf_node);
>
> + unlink_block_successors(block);
> +
> if (jump_instr->type == nir_jump_continue) {
> nir_cf_node *first_node = nir_loop_first_cf_node(loop);
> assert(first_node->type == nir_cf_node_block);
> @@ -887,8 +885,12 @@ handle_jump(nir_block *block)
> if (last_block->successors[1] != NULL)
> unlink_blocks(last_block, after_block);
> }
> + } else if (jump_instr->type == nir_jump_if) {
> + /* Do nothing here for the moment */
> } else {
> assert(jump_instr->type == nir_jump_return);
> +
> + unlink_block_successors(block);
> link_blocks(block, impl->end_block, NULL);
> }
> }
> @@ -1008,26 +1010,9 @@ insert_block_after_block(nir_block *block, nir_block *after, bool has_jump)
> handle_jump(block);
> }
>
> -static void
> -update_if_uses(nir_cf_node *node)
> -{
> - if (node->type != nir_cf_node_if)
> - return;
> -
> - nir_if *if_stmt = nir_cf_node_as_if(node);
> -
> - struct set *if_uses_set = if_stmt->condition.is_ssa ?
> - if_stmt->condition.ssa->if_uses :
> - if_stmt->condition.reg.reg->uses;
> -
> - _mesa_set_add(if_uses_set, if_stmt);
> -}
> -
> void
> nir_cf_node_insert_after(nir_cf_node *node, nir_cf_node *after)
> {
> - update_if_uses(after);
> -
> if (after->type == nir_cf_node_block) {
> /*
> * either node or the one after it must be a basic block, by invariant #2;
> @@ -1073,8 +1058,6 @@ nir_cf_node_insert_after(nir_cf_node *node, nir_cf_node *after)
> void
> nir_cf_node_insert_before(nir_cf_node *node, nir_cf_node *before)
> {
> - update_if_uses(before);
> -
> if (before->type == nir_cf_node_block) {
> nir_block *before_block = nir_cf_node_as_block(before);
>
> @@ -1172,17 +1155,6 @@ cleanup_cf_node(nir_cf_node *node)
> cleanup_cf_node(child);
> foreach_list_typed(nir_cf_node, child, node, &if_stmt->else_list)
> cleanup_cf_node(child);
> -
> - struct set *if_uses;
> - if (if_stmt->condition.is_ssa) {
> - if_uses = if_stmt->condition.ssa->if_uses;
> - } else {
> - if_uses = if_stmt->condition.reg.reg->if_uses;
> - }
> -
> - struct set_entry *entry = _mesa_set_search(if_uses, if_stmt);
> - assert(entry);
> - _mesa_set_remove(if_uses, entry);
> break;
> }
>
> @@ -1652,6 +1624,15 @@ visit_load_const_src(nir_load_const_instr *instr, nir_foreach_src_cb cb,
> }
>
> static bool
> +visit_jump_src(nir_jump_instr *instr, nir_foreach_src_cb cb, void *state)
> +{
> + if (instr->type == nir_jump_if)
> + return visit_src(&instr->if_src, cb, state);
> + else
> + return true;
> +}
> +
> +static bool
> visit_phi_src(nir_phi_instr *instr, nir_foreach_src_cb cb, void *state)
> {
> nir_foreach_phi_src(instr, src) {
> @@ -1714,6 +1695,10 @@ nir_foreach_src(nir_instr *instr, nir_foreach_src_cb cb, void *state)
> if (!visit_load_const_src(nir_instr_as_load_const(instr), cb, state))
> return false;
> break;
> + case nir_instr_type_jump:
> + if (!visit_jump_src(nir_instr_as_jump(instr), cb, state))
> + return false;
> + break;
> case nir_instr_type_phi:
> if (!visit_phi_src(nir_instr_as_phi(instr), cb, state))
> return false;
> @@ -1723,7 +1708,6 @@ nir_foreach_src(nir_instr *instr, nir_foreach_src_cb cb, void *state)
> cb, state))
> return false;
> break;
> - case nir_instr_type_jump:
> case nir_instr_type_ssa_undef:
> return true;
>
> @@ -1846,8 +1830,6 @@ nir_ssa_def_init(nir_instr *instr, nir_ssa_def *def,
> def->parent_instr = instr;
> def->uses = _mesa_set_create(mem_ctx, _mesa_hash_pointer,
> _mesa_key_pointer_equal);
> - def->if_uses = _mesa_set_create(mem_ctx, _mesa_hash_pointer,
> - _mesa_key_pointer_equal);
> def->num_components = num_components;
>
> if (instr->block) {
> @@ -1895,13 +1877,11 @@ nir_ssa_def_rewrite_uses(nir_ssa_def *def, nir_src new_src, void *mem_ctx)
>
> assert(!new_src.is_ssa || def != new_src.ssa);
>
> - struct set *new_uses, *new_if_uses;
> + struct set *new_uses;
> if (new_src.is_ssa) {
> new_uses = new_src.ssa->uses;
> - new_if_uses = new_src.ssa->if_uses;
> } else {
> new_uses = new_src.reg.reg->uses;
> - new_if_uses = new_src.reg.reg->if_uses;
> }
>
> struct set_entry *entry;
> @@ -1912,14 +1892,6 @@ nir_ssa_def_rewrite_uses(nir_ssa_def *def, nir_src new_src, void *mem_ctx)
> nir_foreach_src(instr, ssa_def_rewrite_uses_src, &state);
> _mesa_set_add(new_uses, instr);
> }
> -
> - set_foreach(def->if_uses, entry) {
> - nir_if *if_use = (nir_if *)entry->key;
> -
> - _mesa_set_remove(def->if_uses, entry);
> - nir_src_copy(&if_use->condition, &new_src, mem_ctx);
> - _mesa_set_add(new_if_uses, if_use);
> - }
> }
>
>
> diff --git a/src/glsl/nir/nir.h b/src/glsl/nir/nir.h
> index d5df596..1bfc616 100644
> --- a/src/glsl/nir/nir.h
> +++ b/src/glsl/nir/nir.h
> @@ -400,9 +400,6 @@ typedef struct {
>
> /** set of nir_instr's where this register is defined (written to) */
> struct set *defs;
> -
> - /** set of nir_if's where this register is used as a condition */
> - struct set *if_uses;
> } nir_register;
>
> typedef enum {
> @@ -463,9 +460,6 @@ typedef struct {
> /** set of nir_instr's where this register is used (read from) */
> struct set *uses;
>
> - /** set of nir_if's where this register is used as a condition */
> - struct set *if_uses;
> -
> uint8_t num_components;
> } nir_ssa_def;
>
> @@ -1032,11 +1026,15 @@ typedef enum {
> nir_jump_return,
> nir_jump_break,
> nir_jump_continue,
> + nir_jump_if,
> } nir_jump_type;
>
> typedef struct {
> nir_instr instr;
> nir_jump_type type;
> +
> + /* Only used for if instructions */
> + nir_src if_src;
Have you considered renaming this to something more generic like
"condition"? We might want to use this to support conditional breaks
and continues as well, if they might help us a little with another ...
*cough* ... source language.
> } nir_jump_instr;
>
> /* creates a new SSA variable in an undefined state */
> @@ -1192,7 +1190,6 @@ nir_block_last_instr(nir_block *block)
>
> typedef struct {
> nir_cf_node cf_node;
> - nir_src condition;
>
> struct exec_list then_list; /** < list of nir_cf_node */
> struct exec_list else_list; /** < list of nir_cf_node */
> diff --git a/src/glsl/nir/nir_from_ssa.c b/src/glsl/nir/nir_from_ssa.c
> index c695c95..45cdea1 100644
> --- a/src/glsl/nir/nir_from_ssa.c
> +++ b/src/glsl/nir/nir_from_ssa.c
> @@ -558,7 +558,6 @@ rewrite_ssa_dest(nir_dest *dest, void *void_state)
> }
>
> _mesa_set_destroy(dest->ssa.uses, NULL);
> - _mesa_set_destroy(dest->ssa.if_uses, NULL);
>
> memset(dest, 0, sizeof *dest);
> dest->reg.reg = reg;
> @@ -590,23 +589,6 @@ resolve_registers_block(nir_block *block, void *void_state)
> }
> state->instr = NULL;
>
> - nir_if *following_if = nir_block_get_following_if(block);
> - if (following_if && following_if->condition.is_ssa) {
> - nir_register *reg = get_register_for_ssa_def(following_if->condition.ssa,
> - state);
> - if (reg) {
> - memset(&following_if->condition, 0, sizeof following_if->condition);
> - following_if->condition.reg.reg = reg;
> -
> - _mesa_set_add(reg->if_uses, following_if);
> - } else {
> - /* FIXME: We really shouldn't hit this. We should be doing
> - * constant control flow propagation.
> - */
> - assert(following_if->condition.ssa->parent_instr->type == nir_instr_type_load_const);
> - }
> - }
> -
> return true;
> }
>
> diff --git a/src/glsl/nir/nir_live_variables.c b/src/glsl/nir/nir_live_variables.c
> index 7402dc0..f10d352 100644
> --- a/src/glsl/nir/nir_live_variables.c
> +++ b/src/glsl/nir/nir_live_variables.c
> @@ -200,10 +200,6 @@ nir_live_variables_impl(nir_function_impl *impl)
> memcpy(block->live_in, block->live_out,
> state.bitset_words * sizeof(BITSET_WORD));
>
> - nir_if *following_if = nir_block_get_following_if(block);
> - if (following_if)
> - set_src_live(&following_if->condition, block->live_in);
> -
> nir_foreach_instr_reverse(block, instr) {
> /* Phi nodes are handled seperately so we want to skip them. Since
> * we are going backwards and they are at the beginning, we can just
> diff --git a/src/glsl/nir/nir_lower_to_source_mods.c b/src/glsl/nir/nir_lower_to_source_mods.c
> index d6bf77f..f4283e3 100644
> --- a/src/glsl/nir/nir_lower_to_source_mods.c
> +++ b/src/glsl/nir/nir_lower_to_source_mods.c
> @@ -81,8 +81,7 @@ nir_lower_to_source_mods_block(nir_block *block, void *state)
> alu->src[i].swizzle[j] = parent->src[0].swizzle[alu->src[i].swizzle[j]];
> }
>
> - if (parent->dest.dest.ssa.uses->entries == 0 &&
> - parent->dest.dest.ssa.if_uses->entries == 0)
> + if (parent->dest.dest.ssa.uses->entries == 0)
> nir_instr_remove(&parent->instr);
> }
>
> @@ -124,9 +123,6 @@ nir_lower_to_source_mods_block(nir_block *block, void *state)
> if (nir_op_infos[alu->op].output_type != nir_type_float)
> continue;
>
> - if (alu->dest.dest.ssa.if_uses->entries != 0)
> - continue;
> -
> bool all_children_are_sat = true;
> struct set_entry *entry;
> set_foreach(alu->dest.dest.ssa.uses, entry) {
> diff --git a/src/glsl/nir/nir_opt_copy_propagate.c b/src/glsl/nir/nir_opt_copy_propagate.c
> index ee78e5a..d3361dc 100644
> --- a/src/glsl/nir/nir_opt_copy_propagate.c
> +++ b/src/glsl/nir/nir_opt_copy_propagate.c
> @@ -135,26 +135,12 @@ rewrite_src_instr(nir_src *src, nir_ssa_def *new_def, nir_instr *parent_instr)
> _mesa_set_add(new_def->uses, parent_instr);
> }
>
> -static void
> -rewrite_src_if(nir_if *if_stmt, nir_ssa_def *new_def)
> -{
> - nir_ssa_def *old_def = if_stmt->condition.ssa;
> -
> - if_stmt->condition.ssa = new_def;
> -
> - struct set_entry *entry = _mesa_set_search(old_def->if_uses, if_stmt);
> - assert(entry);
> - _mesa_set_remove(old_def->if_uses, entry);
> -
> - _mesa_set_add(new_def->if_uses, if_stmt);
> -}
> -
> static bool
> -copy_prop_src(nir_src *src, nir_instr *parent_instr, nir_if *parent_if)
> +copy_prop_src(nir_src *src, nir_instr *parent_instr)
> {
> if (!src->is_ssa) {
> if (src->reg.indirect)
> - return copy_prop_src(src, parent_instr, parent_if);
> + return copy_prop_src(src, parent_instr);
> return false;
> }
>
> @@ -170,7 +156,7 @@ copy_prop_src(nir_src *src, nir_instr *parent_instr, nir_if *parent_if)
> * components in its source than it has in its destination. That badly
> * messes up out-of-ssa.
> */
> - if (parent_instr && parent_instr->type == nir_instr_type_phi) {
> + if (parent_instr->type == nir_instr_type_phi) {
> nir_phi_instr *phi = nir_instr_as_phi(parent_instr);
> assert(phi->dest.is_ssa);
> if (phi->dest.ssa.num_components !=
> @@ -178,10 +164,7 @@ copy_prop_src(nir_src *src, nir_instr *parent_instr, nir_if *parent_if)
> return false;
> }
>
> - if (parent_instr)
> - rewrite_src_instr(src, alu_instr->src[0].src.ssa, parent_instr);
> - else
> - rewrite_src_if(parent_if, alu_instr->src[0].src.ssa);
> + rewrite_src_instr(src, alu_instr->src[0].src.ssa, parent_instr);
>
> return true;
> }
> @@ -192,8 +175,7 @@ copy_prop_alu_src(nir_alu_instr *parent_alu_instr, unsigned index)
> nir_alu_src *src = &parent_alu_instr->src[index];
> if (!src->src.is_ssa) {
> if (src->src.reg.indirect)
> - return copy_prop_src(src->src.reg.indirect, &parent_alu_instr->instr,
> - NULL);
> + return copy_prop_src(src->src.reg.indirect, &parent_alu_instr->instr);
> return false;
> }
>
> @@ -248,7 +230,7 @@ static bool
> copy_prop_src_cb(nir_src *src, void *_state)
> {
> copy_prop_state *state = (copy_prop_state *) _state;
> - while (copy_prop_src(src, state->parent_instr, NULL))
> + while (copy_prop_src(src, state->parent_instr))
> state->progress = true;
>
> return true;
> @@ -266,7 +248,7 @@ copy_prop_instr(nir_instr *instr)
> progress = true;
>
> if (!alu_instr->dest.dest.is_ssa && alu_instr->dest.dest.reg.indirect)
> - while (copy_prop_src(alu_instr->dest.dest.reg.indirect, instr, NULL))
> + while (copy_prop_src(alu_instr->dest.dest.reg.indirect, instr))
> progress = true;
>
> return progress;
> @@ -281,12 +263,6 @@ copy_prop_instr(nir_instr *instr)
> }
>
> static bool
> -copy_prop_if(nir_if *if_stmt)
> -{
> - return copy_prop_src(&if_stmt->condition, NULL, if_stmt);
> -}
> -
> -static bool
> copy_prop_block(nir_block *block, void *_state)
> {
> bool *progress = (bool *) _state;
> @@ -296,14 +272,6 @@ copy_prop_block(nir_block *block, void *_state)
> *progress = true;
> }
>
> - if (block->cf_node.node.next != NULL && /* check that we aren't the end node */
> - !nir_cf_node_is_last(&block->cf_node) &&
> - nir_cf_node_next(&block->cf_node)->type == nir_cf_node_if) {
> - nir_if *if_stmt = nir_cf_node_as_if(nir_cf_node_next(&block->cf_node));
> - if (copy_prop_if(if_stmt))
> - *progress = true;
> - }
> -
> return true;
> }
>
> diff --git a/src/glsl/nir/nir_opt_dce.c b/src/glsl/nir/nir_opt_dce.c
> index e0ebdc6..3d3ea3f 100644
> --- a/src/glsl/nir/nir_opt_dce.c
> +++ b/src/glsl/nir/nir_opt_dce.c
> @@ -120,13 +120,6 @@ init_block_cb(nir_block *block, void *_state)
> nir_foreach_instr(block, instr)
> init_instr(instr, worklist);
>
> - nir_if *following_if = nir_block_get_following_if(block);
> - if (following_if) {
> - if (following_if->condition.is_ssa &&
> - !following_if->condition.ssa->parent_instr->pass_flags)
> - worklist_push(worklist, following_if->condition.ssa->parent_instr);
> - }
> -
> return true;
> }
>
> diff --git a/src/glsl/nir/nir_opt_gcm.c b/src/glsl/nir/nir_opt_gcm.c
> index b4f5fd3..d2553b7 100644
> --- a/src/glsl/nir/nir_opt_gcm.c
> +++ b/src/glsl/nir/nir_opt_gcm.c
> @@ -304,18 +304,6 @@ gcm_schedule_late_def(nir_ssa_def *def, void *void_state)
> }
> }
>
> - set_foreach(def->if_uses, entry) {
> - nir_if *if_stmt = (nir_if *)entry->key;
> -
> - /* For if statements, we consider the block to be the one immediately
> - * preceding the if CF node.
> - */
> - nir_block *pred_block =
> - nir_cf_node_as_block(nir_cf_node_prev(&if_stmt->cf_node));
> -
> - lca = nir_dominance_lca(lca, pred_block);
> - }
> -
> /* Some instructions may never be used. We'll just leave them scheduled
> * early and let dead code clean them up.
> */
> diff --git a/src/glsl/nir/nir_opt_global_to_local.c b/src/glsl/nir/nir_opt_global_to_local.c
> index 00db37b..f7b7a04 100644
> --- a/src/glsl/nir/nir_opt_global_to_local.c
> +++ b/src/glsl/nir/nir_opt_global_to_local.c
> @@ -59,17 +59,6 @@ global_to_local(nir_register *reg)
> }
> }
>
> - set_foreach(reg->if_uses, entry) {
> - nir_if *if_stmt = (nir_if *) entry->key;
> - nir_function_impl *if_impl = nir_cf_node_get_function(&if_stmt->cf_node);
> - if (impl != NULL) {
> - if (impl != if_impl)
> - return false;
> - } else {
> - impl = if_impl;
> - }
> - }
> -
> if (impl == NULL) {
> /* this instruction is never used/defined, delete it */
> nir_reg_remove(reg);
> diff --git a/src/glsl/nir/nir_opt_peephole_select.c b/src/glsl/nir/nir_opt_peephole_select.c
> index ab08f28..d404658 100644
> --- a/src/glsl/nir/nir_opt_peephole_select.c
> +++ b/src/glsl/nir/nir_opt_peephole_select.c
> @@ -71,10 +71,6 @@ are_all_move_to_phi(nir_block *block)
> 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;
> -
> /* 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) {
> @@ -103,11 +99,11 @@ nir_opt_peephole_select_block(nir_block *block, void *void_state)
> if (nir_cf_node_is_first(&block->cf_node))
> return true;
>
> - nir_cf_node *prev_node = nir_cf_node_prev(&block->cf_node);
> - if (prev_node->type != nir_cf_node_if)
> + nir_cf_node *if_node = nir_cf_node_prev(&block->cf_node);
> + if (if_node->type != nir_cf_node_if)
> return true;
>
> - nir_if *if_stmt = nir_cf_node_as_if(prev_node);
> + nir_if *if_stmt = nir_cf_node_as_if(if_node);
> nir_cf_node *then_node = nir_if_first_then_node(if_stmt);
> nir_cf_node *else_node = nir_if_first_else_node(if_stmt);
>
> @@ -129,13 +125,21 @@ nir_opt_peephole_select_block(nir_block *block, void *void_state)
> * selects.
> */
>
> + nir_cf_node *prev_node = nir_cf_node_prev(if_node);
> + assert(prev_node->type == nir_cf_node_block);
> + nir_block *prev_block = nir_cf_node_as_block(prev_node);
> + nir_instr *if_instr = nir_block_last_instr(prev_block);
> + assert(if_instr->type == nir_instr_type_jump);
> + nir_jump_instr *if_jump_instr = nir_instr_as_jump(if_instr);
> + assert(if_jump_instr->type == nir_jump_if);
> +
> nir_foreach_instr_safe(block, instr) {
> if (instr->type != nir_instr_type_phi)
> break;
>
> nir_phi_instr *phi = nir_instr_as_phi(instr);
> nir_alu_instr *sel = nir_alu_instr_create(state->mem_ctx, nir_op_bcsel);
> - nir_src_copy(&sel->src[0].src, &if_stmt->condition, state->mem_ctx);
> + nir_src_copy(&sel->src[0].src, &if_jump_instr->if_src, state->mem_ctx);
> /* Splat the condition to all channels */
> memset(sel->src[0].swizzle, 0, sizeof sel->src[0].swizzle);
>
> @@ -172,6 +176,7 @@ nir_opt_peephole_select_block(nir_block *block, void *void_state)
> nir_instr_remove(&phi->instr);
> }
>
> + nir_instr_remove(&if_jump_instr->instr);
> nir_cf_node_remove(&if_stmt->cf_node);
> state->progress = true;
>
> diff --git a/src/glsl/nir/nir_print.c b/src/glsl/nir/nir_print.c
> index 6a3c6a0..44468eb 100644
> --- a/src/glsl/nir/nir_print.c
> +++ b/src/glsl/nir/nir_print.c
> @@ -538,6 +538,11 @@ print_jump_instr(nir_jump_instr *instr, FILE *fp)
> case nir_jump_return:
> fprintf(fp, "return");
> break;
> +
> + case nir_jump_if:
> + fprintf(fp, "if ");
> + print_src(&instr->if_src, fp);
> + break;
> }
> }
>
> @@ -682,9 +687,7 @@ static void
> print_if(nir_if *if_stmt, print_var_state *state, unsigned tabs, FILE *fp)
> {
> print_tabs(tabs, fp);
> - fprintf(fp, "if ");
> - print_src(&if_stmt->condition, fp);
> - fprintf(fp, " {\n");
> + fprintf(fp, "if {\n");
> foreach_list_typed(nir_cf_node, node, node, &if_stmt->then_list) {
> print_cf_node(node, state, tabs + 1, fp);
> }
> diff --git a/src/glsl/nir/nir_to_ssa.c b/src/glsl/nir/nir_to_ssa.c
> index 47cf453..ebf7fc9 100644
> --- a/src/glsl/nir/nir_to_ssa.c
> +++ b/src/glsl/nir/nir_to_ssa.c
> @@ -143,7 +143,6 @@ typedef struct {
> reg_state *states;
> void *mem_ctx;
> nir_instr *parent_instr;
> - nir_if *parent_if;
> nir_function_impl *impl;
>
> /* map from SSA value -> original register */
> @@ -193,10 +192,8 @@ rewrite_use(nir_src *src, void *_state)
> src->is_ssa = true;
> src->ssa = get_ssa_src(src->reg.reg, state);
>
> - if (state->parent_instr)
> - _mesa_set_add(src->ssa->uses, state->parent_instr);
> - else
> - _mesa_set_add(src->ssa->if_uses, state->parent_if);
> + _mesa_set_add(src->ssa->uses, state->parent_instr);
> +
> return true;
> }
>
> @@ -433,15 +430,6 @@ rewrite_block(nir_block *block, rewrite_state *state)
> rewrite_instr_forward(instr, state);
> }
>
> - if (block != state->impl->end_block &&
> - !nir_cf_node_is_last(&block->cf_node) &&
> - nir_cf_node_next(&block->cf_node)->type == nir_cf_node_if) {
> - nir_if *if_stmt = nir_cf_node_as_if(nir_cf_node_next(&block->cf_node));
> - state->parent_instr = NULL;
> - state->parent_if = if_stmt;
> - rewrite_use(&if_stmt->condition, state);
> - }
> -
> if (block->successors[0])
> rewrite_phi_sources(block->successors[0], block, state);
> if (block->successors[1])
> diff --git a/src/glsl/nir/nir_validate.c b/src/glsl/nir/nir_validate.c
> index a3fe9d6..31b65f0 100644
> --- a/src/glsl/nir/nir_validate.c
> +++ b/src/glsl/nir/nir_validate.c
> @@ -46,7 +46,7 @@ typedef struct {
> * equivalent to the uses and defs in nir_register, but built up by the
> * validator. At the end, we verify that the sets have the same entries.
> */
> - struct set *uses, *if_uses, *defs;
> + struct set *uses, *defs;
> nir_function_impl *where_defined; /* NULL for global registers */
> } reg_validate_state;
>
> @@ -55,7 +55,7 @@ typedef struct {
> * equivalent to the uses in nir_ssa_def, but built up by the validator.
> * At the end, we verify that the sets have the same entries.
> */
> - struct set *uses, *if_uses;
> + struct set *uses;
> nir_function_impl *where_defined;
> } ssa_def_validate_state;
>
> @@ -107,16 +107,8 @@ validate_reg_src(nir_reg_src *src, validate_state *state)
>
> reg_validate_state *reg_state = (reg_validate_state *) entry->data;
>
> - if (state->instr) {
> - _mesa_set_add(reg_state->uses, state->instr);
> -
> - assert(_mesa_set_search(src->reg->uses, state->instr));
> - } else {
> - assert(state->if_stmt);
> - _mesa_set_add(reg_state->if_uses, state->if_stmt);
> -
> - assert(_mesa_set_search(src->reg->if_uses, state->if_stmt));
> - }
> + _mesa_set_add(reg_state->uses, state->instr);
> + assert(_mesa_set_search(src->reg->uses, state->instr));
>
> if (!src->reg->is_global) {
> assert(reg_state->where_defined == state->impl &&
> @@ -149,16 +141,8 @@ validate_ssa_src(nir_ssa_def *def, validate_state *state)
> assert(def_state->where_defined == state->impl &&
> "using an SSA value defined in a different function");
>
> - if (state->instr) {
> - _mesa_set_add(def_state->uses, state->instr);
> -
> - assert(_mesa_set_search(def->uses, state->instr));
> - } else {
> - assert(state->if_stmt);
> - _mesa_set_add(def_state->if_uses, state->if_stmt);
> -
> - assert(_mesa_set_search(def->if_uses, state->if_stmt));
> - }
> + _mesa_set_add(def_state->uses, state->instr);
> + assert(_mesa_set_search(def->uses, state->instr));
>
> /* TODO validate that the use is dominated by the definition */
> }
> @@ -243,8 +227,6 @@ validate_ssa_def(nir_ssa_def *def, validate_state *state)
> def_state->where_defined = state->impl;
> def_state->uses = _mesa_set_create(def_state, _mesa_hash_pointer,
> _mesa_key_pointer_equal);
> - def_state->if_uses = _mesa_set_create(def_state, _mesa_hash_pointer,
> - _mesa_key_pointer_equal);
> _mesa_hash_table_insert(state->ssa_defs, def, def_state);
> }
>
> @@ -457,6 +439,18 @@ validate_ssa_undef_instr(nir_ssa_undef_instr *instr, validate_state *state)
> }
>
> static void
> +validate_jump_instr(nir_jump_instr *instr, validate_state *state)
> +{
> + assert(&instr->instr == nir_block_last_instr(instr->instr.block));
> +
> + if (instr->type == nir_jump_if) {
> + validate_src(&instr->if_src, state);
> + nir_cf_node *if_node = nir_cf_node_next(&instr->instr.block->cf_node);
> + assert(if_node->type == nir_cf_node_if);
> + }
> +}
> +
> +static void
> validate_phi_instr(nir_phi_instr *instr, validate_state *state)
> {
> /*
> @@ -508,6 +502,7 @@ validate_instr(nir_instr *instr, validate_state *state)
> break;
>
> case nir_instr_type_jump:
> + validate_jump_instr(nir_instr_as_jump(instr), state);
> break;
>
> default:
> @@ -588,8 +583,18 @@ validate_block(nir_block *block, validate_state *state)
> }
>
> if (!exec_list_is_empty(&block->instr_list) &&
> - nir_block_last_instr(block)->type == nir_instr_type_jump)
> - assert(block->successors[1] == NULL);
> + nir_block_last_instr(block)->type == nir_instr_type_jump) {
> + switch (nir_instr_as_jump(nir_block_last_instr(block))->type) {
> + case nir_jump_break:
> + case nir_jump_continue:
> + case nir_jump_return:
> + assert(block->successors[1] == NULL);
> + break;
> + case nir_jump_if:
> + assert(nir_cf_node_next(&block->cf_node)->type == nir_cf_node_if);
> + break;
> + }
> + }
> }
>
> static void
> @@ -607,12 +612,14 @@ validate_if(nir_if *if_stmt, validate_state *state)
> assert(&prev_block->successors[1]->cf_node ==
> nir_if_first_else_node(if_stmt));
>
> + nir_instr *if_instr = nir_block_last_instr(prev_block);
> + assert(if_instr->type == nir_instr_type_jump);
> + assert(nir_instr_as_jump(if_instr)->type == nir_jump_if);
> +
> assert(!exec_node_is_tail_sentinel(if_stmt->cf_node.node.next));
> nir_cf_node *next_node = nir_cf_node_next(&if_stmt->cf_node);
> assert(next_node->type == nir_cf_node_block);
>
> - validate_src(&if_stmt->condition, state);
> -
> assert(!exec_list_is_empty(&if_stmt->then_list));
> assert(!exec_list_is_empty(&if_stmt->else_list));
>
> @@ -700,8 +707,6 @@ prevalidate_reg_decl(nir_register *reg, bool is_global, validate_state *state)
> reg_validate_state *reg_state = ralloc(state->regs, reg_validate_state);
> reg_state->uses = _mesa_set_create(reg_state, _mesa_hash_pointer,
> _mesa_key_pointer_equal);
> - reg_state->if_uses = _mesa_set_create(reg_state, _mesa_hash_pointer,
> - _mesa_key_pointer_equal);
> reg_state->defs = _mesa_set_create(reg_state, _mesa_hash_pointer,
> _mesa_key_pointer_equal);
>
> @@ -732,21 +737,6 @@ postvalidate_reg_decl(nir_register *reg, validate_state *state)
> abort();
> }
>
> - if (reg_state->if_uses->entries != reg->if_uses->entries) {
> - printf("extra entries in register if_uses:\n");
> - struct set_entry *entry;
> - set_foreach(reg->if_uses, entry) {
> - struct set_entry *entry2 =
> - _mesa_set_search(reg_state->if_uses, entry->key);
> -
> - if (entry2 == NULL) {
> - printf("%p\n", entry->key);
> - }
> - }
> -
> - abort();
> - }
> -
> if (reg_state->defs->entries != reg->defs->entries) {
> printf("extra entries in register defs:\n");
> struct set_entry *entry;
> @@ -801,21 +791,6 @@ postvalidate_ssa_def(nir_ssa_def *def, void *void_state)
> abort();
> }
>
> - if (def_state->if_uses->entries != def->if_uses->entries) {
> - printf("extra entries in SSA def uses:\n");
> - struct set_entry *entry;
> - set_foreach(def->if_uses, entry) {
> - struct set_entry *entry2 =
> - _mesa_set_search(def_state->if_uses, entry->key);
> -
> - if (entry2 == NULL) {
> - printf("%p\n", entry->key);
> - }
> - }
> -
> - abort();
> - }
> -
> return true;
> }
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> index 388e636..a3841b9 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> @@ -440,12 +440,6 @@ fs_visitor::nir_emit_cf_list(exec_list *list)
> void
> fs_visitor::nir_emit_if(nir_if *if_stmt)
> {
> - /* first, put the condition into f0 */
> - fs_inst *inst = emit(MOV(reg_null_d,
> - retype(get_nir_src(if_stmt->condition),
> - BRW_REGISTER_TYPE_UD)));
> - inst->conditional_mod = BRW_CONDITIONAL_NZ;
> -
> emit(IF(BRW_PREDICATE_NORMAL));
>
> nir_emit_cf_list(&if_stmt->then_list);
> @@ -1759,6 +1753,15 @@ fs_visitor::nir_emit_jump(nir_jump_instr *instr)
> case nir_jump_continue:
> emit(BRW_OPCODE_CONTINUE);
> break;
> + case nir_jump_if: {
> + /* Move the condition to the flag register. The nir_emit_if (which
> + * should immediately follow this) will use the flag register.
> + */
> + fs_reg cond = retype(get_nir_src(instr->if_src), BRW_REGISTER_TYPE_UD);
> + fs_inst *inst = emit(MOV(reg_null_d, cond));
> + inst->conditional_mod = BRW_CONDITIONAL_NZ;
> + break;
> + }
> case nir_jump_return:
> default:
> unreachable("unknown jump");
> --
> 2.3.0
>
More information about the mesa-dev
mailing list