From 370d68babcbb8b2e80fb866429e792ee3c43ffeb Mon Sep 17 00:00:00 2001 From: Jason Ekstrand Date: Thu, 2 Mar 2017 16:25:59 -0800 Subject: [PATCH] nir/validate: Validate that bit sizes and components always match We've always required bit sizes to match but the rules for number of components have been a bit loose. You've never been allowed to source from something with less components than you consume, but more has always been fine. This changes the validator to require that they match exactly. The fact that they don't always match has been a source of confusion in NIR for quite some time and it's time we got rid of it. Reviewed-by: Eric Anholt Reviewed-by: Kenneth Graunke Reviewed-by: Connor Abbott --- src/compiler/nir/nir_validate.c | 101 ++++++++++++++++++++------------ 1 file changed, 63 insertions(+), 38 deletions(-) diff --git a/src/compiler/nir/nir_validate.c b/src/compiler/nir/nir_validate.c index 15ba65f2b12..16efcb2356c 100644 --- a/src/compiler/nir/nir_validate.c +++ b/src/compiler/nir/nir_validate.c @@ -126,10 +126,12 @@ log_error(validate_state *state, const char *cond, const char *file, int line) log_error(state, #cond, __FILE__, __LINE__); \ } while (0) -static void validate_src(nir_src *src, validate_state *state); +static void validate_src(nir_src *src, validate_state *state, + unsigned bit_size, unsigned num_components); static void -validate_reg_src(nir_src *src, validate_state *state) +validate_reg_src(nir_src *src, validate_state *state, + unsigned bit_size, unsigned num_components) { validate_assert(state, src->reg.reg != NULL); @@ -151,6 +153,13 @@ validate_reg_src(nir_src *src, validate_state *state) "using a register declared in a different function"); } + if (!src->reg.reg->is_packed) { + if (bit_size) + validate_assert(state, src->reg.reg->bit_size == bit_size); + if (num_components) + validate_assert(state, src->reg.reg->num_components == num_components); + } + validate_assert(state, (src->reg.reg->num_array_elems == 0 || src->reg.base_offset < src->reg.reg->num_array_elems) && "definitely out-of-bounds array access"); @@ -160,12 +169,13 @@ validate_reg_src(nir_src *src, validate_state *state) validate_assert(state, (src->reg.indirect->is_ssa || src->reg.indirect->reg.indirect == NULL) && "only one level of indirection allowed"); - validate_src(src->reg.indirect, state); + validate_src(src->reg.indirect, state, 32, 1); } } static void -validate_ssa_src(nir_src *src, validate_state *state) +validate_ssa_src(nir_src *src, validate_state *state, + unsigned bit_size, unsigned num_components) { validate_assert(state, src->ssa != NULL); @@ -188,11 +198,17 @@ validate_ssa_src(nir_src *src, validate_state *state) _mesa_set_add(def_state->if_uses, src); } + if (bit_size) + validate_assert(state, src->ssa->bit_size == bit_size); + if (num_components) + validate_assert(state, src->ssa->num_components == num_components); + /* TODO validate that the use is dominated by the definition */ } static void -validate_src(nir_src *src, validate_state *state) +validate_src(nir_src *src, validate_state *state, + unsigned bit_size, unsigned num_components) { if (state->instr) validate_assert(state, src->parent_instr == state->instr); @@ -200,9 +216,9 @@ validate_src(nir_src *src, validate_state *state) validate_assert(state, src->parent_if == state->if_stmt); if (src->is_ssa) - validate_ssa_src(src, state); + validate_ssa_src(src, state, bit_size, num_components); else - validate_reg_src(src, state); + validate_reg_src(src, state, bit_size, num_components); } static void @@ -247,11 +263,12 @@ validate_alu_src(nir_alu_instr *instr, unsigned index, validate_state *state) } } - validate_src(&src->src, state); + validate_src(&src->src, state, 0, 0); } static void -validate_reg_dest(nir_reg_dest *dest, validate_state *state) +validate_reg_dest(nir_reg_dest *dest, validate_state *state, + unsigned bit_size, unsigned num_components) { validate_assert(state, dest->reg != NULL); @@ -270,6 +287,13 @@ validate_reg_dest(nir_reg_dest *dest, validate_state *state) "writing to a register declared in a different function"); } + if (!dest->reg->is_packed) { + if (bit_size) + validate_assert(state, dest->reg->bit_size == bit_size); + if (num_components) + validate_assert(state, dest->reg->num_components == num_components); + } + validate_assert(state, (dest->reg->num_array_elems == 0 || dest->base_offset < dest->reg->num_array_elems) && "definitely out-of-bounds array access"); @@ -278,7 +302,7 @@ validate_reg_dest(nir_reg_dest *dest, validate_state *state) validate_assert(state, dest->reg->num_array_elems != 0); validate_assert(state, (dest->indirect->is_ssa || dest->indirect->reg.indirect == NULL) && "only one level of indirection allowed"); - validate_src(dest->indirect, state); + validate_src(dest->indirect, state, 32, 1); } } @@ -307,12 +331,18 @@ validate_ssa_def(nir_ssa_def *def, validate_state *state) } static void -validate_dest(nir_dest *dest, validate_state *state) +validate_dest(nir_dest *dest, validate_state *state, + unsigned bit_size, unsigned num_components) { - if (dest->is_ssa) + if (dest->is_ssa) { + if (bit_size) + validate_assert(state, dest->ssa.bit_size == bit_size); + if (num_components) + validate_assert(state, dest->ssa.num_components == num_components); validate_ssa_def(&dest->ssa, state); - else - validate_reg_dest(&dest->reg, state); + } else { + validate_reg_dest(&dest->reg, state, bit_size, num_components); + } } static void @@ -350,7 +380,7 @@ validate_alu_dest(nir_alu_instr *instr, validate_state *state) validate_assert(state, nir_alu_type_get_type_size(type) == 0 || nir_alu_type_get_type_size(type) == bit_size); - validate_dest(&dest->dest, state); + validate_dest(&dest->dest, state, 0, 0); } static void @@ -377,7 +407,7 @@ validate_deref_chain(nir_deref *deref, validate_state *state) validate_assert(state, deref->type == glsl_get_array_element(parent->type)); if (nir_deref_as_array(deref)->deref_array_type == nir_deref_array_type_indirect) - validate_src(&nir_deref_as_array(deref)->indirect, state); + validate_src(&nir_deref_as_array(deref)->indirect, state, 32, 1); break; case nir_deref_type_struct: @@ -426,6 +456,14 @@ validate_deref_var(void *parent_mem_ctx, nir_deref_var *deref, validate_state *s static void validate_intrinsic_instr(nir_intrinsic_instr *instr, validate_state *state) { + unsigned bit_size = 0; + if (instr->intrinsic == nir_intrinsic_load_var || + instr->intrinsic == nir_intrinsic_store_var) { + const struct glsl_type *type = + nir_deref_tail(&instr->variables[0]->deref)->type; + bit_size = glsl_get_bit_size(type); + } + unsigned num_srcs = nir_intrinsic_infos[instr->intrinsic].num_srcs; for (unsigned i = 0; i < num_srcs; i++) { unsigned components_read = @@ -435,13 +473,7 @@ validate_intrinsic_instr(nir_intrinsic_instr *instr, validate_state *state) validate_assert(state, components_read > 0); - if (instr->src[i].is_ssa) { - validate_assert(state, components_read <= instr->src[i].ssa->num_components); - } else if (!instr->src[i].reg.reg->is_packed) { - validate_assert(state, components_read <= instr->src[i].reg.reg->num_components); - } - - validate_src(&instr->src[i], state); + validate_src(&instr->src[i], state, bit_size, components_read); } unsigned num_vars = nir_intrinsic_infos[instr->intrinsic].num_variables; @@ -457,13 +489,7 @@ validate_intrinsic_instr(nir_intrinsic_instr *instr, validate_state *state) validate_assert(state, components_written > 0); - if (instr->dest.is_ssa) { - validate_assert(state, components_written <= instr->dest.ssa.num_components); - } else if (!instr->dest.reg.reg->is_packed) { - validate_assert(state, components_written <= instr->dest.reg.reg->num_components); - } - - validate_dest(&instr->dest, state); + validate_dest(&instr->dest, state, bit_size, components_written); } switch (instr->intrinsic) { @@ -511,7 +537,8 @@ validate_tex_instr(nir_tex_instr *instr, validate_state *state) for (unsigned i = 0; i < instr->num_srcs; i++) { validate_assert(state, !src_type_seen[instr->src[i].src_type]); src_type_seen[instr->src[i].src_type] = true; - validate_src(&instr->src[i].src, state); + validate_src(&instr->src[i].src, state, + 0, nir_tex_instr_src_size(instr, i)); } if (instr->texture != NULL) @@ -520,7 +547,7 @@ validate_tex_instr(nir_tex_instr *instr, validate_state *state) if (instr->sampler != NULL) validate_deref_var(instr, instr->sampler, state); - validate_dest(&instr->dest, state); + validate_dest(&instr->dest, state, 0, nir_tex_instr_dest_size(instr)); } static void @@ -561,7 +588,7 @@ validate_phi_instr(nir_phi_instr *instr, validate_state *state) * basic blocks, to avoid validating an SSA use before its definition. */ - validate_dest(&instr->dest, state); + validate_dest(&instr->dest, state, 0, 0); exec_list_validate(&instr->srcs); validate_assert(state, exec_list_length(&instr->srcs) == @@ -626,10 +653,8 @@ validate_phi_src(nir_phi_instr *instr, nir_block *pred, validate_state *state) nir_foreach_phi_src(src, instr) { if (src->pred == pred) { validate_assert(state, src->src.is_ssa); - validate_assert(state, src->src.ssa->num_components == - instr->dest.ssa.num_components); - - validate_src(&src->src, state); + validate_src(&src->src, state, instr->dest.ssa.bit_size, + instr->dest.ssa.num_components); state->instr = NULL; return; } @@ -777,7 +802,7 @@ validate_if(nir_if *if_stmt, validate_state *state) nir_cf_node *next_node = nir_cf_node_next(&if_stmt->cf_node); validate_assert(state, next_node->type == nir_cf_node_block); - validate_src(&if_stmt->condition, state); + validate_src(&if_stmt->condition, state, 32, 1); validate_assert(state, !exec_list_is_empty(&if_stmt->then_list)); validate_assert(state, !exec_list_is_empty(&if_stmt->else_list)); -- 2.30.2