[Mesa-dev] [PATCH 1/4] glsl: make max array trackers ints and use -1 as base.
Ilia Mirkin
imirkin at alum.mit.edu
Mon May 23 20:40:35 UTC 2016
On Thu, May 19, 2016 at 11:47 PM, Dave Airlie <airlied at gmail.com> wrote:
> From: Dave Airlie <airlied at redhat.com>
>
> This fixes a bug that breaks cull distances. The problem
> is the max array accessors can't tell the difference between
> an never accessed unsized array and an accessed at location 0
> unsized array. This leads to converting an undeclared unused
> gl_ClipDistance inside or outside gl_PerVertex to a size 1
> array. However we need to the number of active clip distances
> to work out the starting point for the cull distances, and
> this offset by one when it's not being used isn't possible
> to distinguish from the case were only the first element is
> accessed. I tried to use ->used for this, but that doesn't
> work when gl_ClipDistance is part of an interface block.
>
> So this changes things so that max_array_access is an int
> and initialised to -1. This also allows unsized arrays to
> proceed further than that could before, but we really shouldn't
> mind as they will get eliminated if nothing uses them later.
>
> For initialised uniforms we no longer change their array
> size at runtime, if these are unused they will get eliminated
> eventually.
>
> Signed-off-by: Dave Airlie <airlied at redhat.com>
> ---
> src/compiler/glsl/ast_array_index.cpp | 4 ++--
> src/compiler/glsl/ast_to_hir.cpp | 8 ++++----
> src/compiler/glsl/ir.cpp | 2 +-
> src/compiler/glsl/ir.h | 15 +++++++++------
> src/compiler/glsl/ir_clone.cpp | 2 +-
> src/compiler/glsl/ir_validate.cpp | 6 +++---
> src/compiler/glsl/link_functions.cpp | 4 ++--
> src/compiler/glsl/link_interface_blocks.cpp | 6 ------
> src/compiler/glsl/linker.cpp | 14 +++++++-------
> src/mesa/main/ff_fragment_shader.cpp | 6 +++---
> 10 files changed, 32 insertions(+), 35 deletions(-)
>
> diff --git a/src/compiler/glsl/ast_array_index.cpp b/src/compiler/glsl/ast_array_index.cpp
> index 69322cf..2e36035 100644
> --- a/src/compiler/glsl/ast_array_index.cpp
> +++ b/src/compiler/glsl/ast_array_index.cpp
> @@ -92,12 +92,12 @@ update_max_array_access(ir_rvalue *ir, int idx, YYLTYPE *loc,
> deref_record->record->type->field_index(deref_record->field);
> assert(field_index < deref_var->var->get_interface_type()->length);
>
> - unsigned *const max_ifc_array_access =
> + int *const max_ifc_array_access =
> deref_var->var->get_max_ifc_array_access();
>
> assert(max_ifc_array_access != NULL);
>
> - if (idx > (int)max_ifc_array_access[field_index]) {
> + if (idx > max_ifc_array_access[field_index]) {
> max_ifc_array_access[field_index] = idx;
>
> /* Check whether this access will, as a side effect, implicitly
> diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp
> index ecfe684..1455bdf 100644
> --- a/src/compiler/glsl/ast_to_hir.cpp
> +++ b/src/compiler/glsl/ast_to_hir.cpp
> @@ -976,7 +976,7 @@ do_assignment(exec_list *instructions, struct _mesa_glsl_parse_state *state,
>
> assert(var != NULL);
>
> - if (var->data.max_array_access >= unsigned(rhs->type->array_size())) {
> + if (var->data.max_array_access >= rhs->type->array_size()) {
> /* FINISHME: This should actually log the location of the RHS. */
> _mesa_glsl_error(& lhs_loc, state, "array size must be > %u due to "
> "previous access",
> @@ -3858,7 +3858,7 @@ get_variable_being_redeclared(ir_variable *var, YYLTYPE loc,
> * FINISHME: required or not.
> */
>
> - const unsigned size = unsigned(var->type->array_size());
> + const int size = var->type->array_size();
> check_builtin_array_max_size(var->name, size, loc, state);
> if ((size > 0) && (size <= earlier->data.max_array_access)) {
> _mesa_glsl_error(& loc, state, "array size must be > %u due to "
> @@ -7711,7 +7711,7 @@ ast_tcs_output_layout::hir(exec_list *instructions,
> if (!var->type->is_unsized_array() || var->data.patch)
> continue;
>
> - if (var->data.max_array_access >= num_vertices) {
> + if (var->data.max_array_access >= (int)num_vertices) {
> _mesa_glsl_error(&loc, state,
> "this tessellation control shader output layout "
> "specifies %u vertices, but an access to element "
> @@ -7772,7 +7772,7 @@ ast_gs_input_layout::hir(exec_list *instructions,
> */
>
> if (var->type->is_unsized_array()) {
> - if (var->data.max_array_access >= num_vertices) {
> + if (var->data.max_array_access >= (int)num_vertices) {
> _mesa_glsl_error(&loc, state,
> "this geometry shader input layout implies %u"
> " vertices, but an access to element %u of input"
> diff --git a/src/compiler/glsl/ir.cpp b/src/compiler/glsl/ir.cpp
> index 9637d7a..5bb3ac3 100644
> --- a/src/compiler/glsl/ir.cpp
> +++ b/src/compiler/glsl/ir.cpp
> @@ -1668,7 +1668,7 @@ ir_variable::ir_variable(const struct glsl_type *type, const char *name,
> this->data.how_declared = ir_var_declared_normally;
> this->data.mode = mode;
> this->data.interpolation = INTERP_QUALIFIER_NONE;
> - this->data.max_array_access = 0;
> + this->data.max_array_access = -1;
> this->data.offset = 0;
> this->data.precision = GLSL_PRECISION_NONE;
> this->data.image_read_only = false;
> diff --git a/src/compiler/glsl/ir.h b/src/compiler/glsl/ir.h
> index 6e0dc0b..4915f60 100644
> --- a/src/compiler/glsl/ir.h
> +++ b/src/compiler/glsl/ir.h
> @@ -484,7 +484,10 @@ public:
> this->interface_type = type;
> if (this->is_interface_instance()) {
> this->u.max_ifc_array_access =
> - rzalloc_array(this, unsigned, type->length);
> + rzalloc_array(this, int, type->length);
This can just be ralloc_array now that you initialize it. Also you
could do memset(-1), but that's probably a little hacky. With the
change to ralloc_array, this patch is
Reviewed-by: Ilia Mirkin <imirkin at alum.mit.edu>
BTW, I notice that you didn't update
src/compiler/glsl/lower_tess_level.cpp:
new_tess_level_outer_var->data.max_array_access = 0;
src/compiler/glsl/lower_tess_level.cpp:
new_tess_level_inner_var->data.max_array_access = 0;
but that seems OK, since they end up as non-array types in the first
place and that setting can probably go.
> + for (unsigned i = 0; i < type->length; i++) {
> + this->u.max_ifc_array_access[i] = -1;
> + }
> }
> }
>
> @@ -520,7 +523,7 @@ public:
> * zero.
> */
> for (unsigned i = 0; i < this->interface_type->length; i++)
> - assert(this->u.max_ifc_array_access[i] == 0);
> + assert(this->u.max_ifc_array_access[i] == -1);
> #endif
> ralloc_free(this->u.max_ifc_array_access);
> this->u.max_ifc_array_access = NULL;
> @@ -540,7 +543,7 @@ public:
> * A "set" function is not needed because the array is dynmically allocated
> * as necessary.
> */
> - inline unsigned *get_max_ifc_array_access()
> + inline int *get_max_ifc_array_access()
> {
> assert(this->data._num_state_slots == 0);
> return this->u.max_ifc_array_access;
> @@ -888,9 +891,9 @@ public:
> /**
> * Highest element accessed with a constant expression array index
> *
> - * Not used for non-array variables.
> + * Not used for non-array variables. -1 is never accessed.
> */
> - unsigned max_array_access;
> + int max_array_access;
>
> /**
> * Transform feedback buffer.
> @@ -938,7 +941,7 @@ private:
> * For variables whose type is not an interface block, this pointer is
> * NULL.
> */
> - unsigned *max_ifc_array_access;
> + int *max_ifc_array_access;
>
> /**
> * Built-in state that backs this uniform
> diff --git a/src/compiler/glsl/ir_clone.cpp b/src/compiler/glsl/ir_clone.cpp
> index 43ffffb..60d1526 100644
> --- a/src/compiler/glsl/ir_clone.cpp
> +++ b/src/compiler/glsl/ir_clone.cpp
> @@ -46,7 +46,7 @@ ir_variable::clone(void *mem_ctx, struct hash_table *ht) const
> var->data.max_array_access = this->data.max_array_access;
> if (this->is_interface_instance()) {
> var->u.max_ifc_array_access =
> - rzalloc_array(var, unsigned, this->interface_type->length);
> + rzalloc_array(var, int, this->interface_type->length);
> memcpy(var->u.max_ifc_array_access, this->u.max_ifc_array_access,
> this->interface_type->length * sizeof(unsigned));
> }
> diff --git a/src/compiler/glsl/ir_validate.cpp b/src/compiler/glsl/ir_validate.cpp
> index 2ec5a3f..9d05e7e 100644
> --- a/src/compiler/glsl/ir_validate.cpp
> +++ b/src/compiler/glsl/ir_validate.cpp
> @@ -727,7 +727,7 @@ ir_validate::visit(ir_variable *ir)
> * to be out of bounds.
> */
> if (ir->type->array_size() > 0) {
> - if (ir->data.max_array_access >= ir->type->length) {
> + if (ir->data.max_array_access >= (int)ir->type->length) {
> printf("ir_variable has maximum access out of bounds (%d vs %d)\n",
> ir->data.max_array_access, ir->type->length - 1);
> ir->print();
> @@ -744,12 +744,12 @@ ir_validate::visit(ir_variable *ir)
> ir->get_interface_type()->fields.structure;
> for (unsigned i = 0; i < ir->get_interface_type()->length; i++) {
> if (fields[i].type->array_size() > 0) {
> - const unsigned *const max_ifc_array_access =
> + const int *const max_ifc_array_access =
> ir->get_max_ifc_array_access();
>
> assert(max_ifc_array_access != NULL);
>
> - if (max_ifc_array_access[i] >= fields[i].type->length) {
> + if (max_ifc_array_access[i] >= (int)fields[i].type->length) {
> printf("ir_variable has maximum access out of bounds for "
> "field %s (%d vs %d)\n", fields[i].name,
> max_ifc_array_access[i], fields[i].type->length);
> diff --git a/src/compiler/glsl/link_functions.cpp b/src/compiler/glsl/link_functions.cpp
> index 537f4dc..4e10287 100644
> --- a/src/compiler/glsl/link_functions.cpp
> +++ b/src/compiler/glsl/link_functions.cpp
> @@ -245,9 +245,9 @@ public:
> /* Similarly, we need implicit sizes of arrays within interface
> * blocks to be sized by the maximal access in *any* shader.
> */
> - unsigned *const linked_max_ifc_array_access =
> + int *const linked_max_ifc_array_access =
> var->get_max_ifc_array_access();
> - unsigned *const ir_max_ifc_array_access =
> + int *const ir_max_ifc_array_access =
> ir->var->get_max_ifc_array_access();
>
> assert(linked_max_ifc_array_access != NULL);
> diff --git a/src/compiler/glsl/link_interface_blocks.cpp b/src/compiler/glsl/link_interface_blocks.cpp
> index 2607259..4eda097 100644
> --- a/src/compiler/glsl/link_interface_blocks.cpp
> +++ b/src/compiler/glsl/link_interface_blocks.cpp
> @@ -154,12 +154,6 @@ static bool
> interstage_match(struct gl_shader_program *prog, ir_variable *producer,
> ir_variable *consumer, bool extra_array_level)
> {
> - /* Unsized arrays should not occur during interstage linking. They
> - * should have all been assigned a size by link_intrastage_shaders.
> - */
> - assert(!consumer->type->is_unsized_array());
> - assert(!producer->type->is_unsized_array());
> -
> /* Types must match. */
> if (consumer->get_interface_type() != producer->get_interface_type()) {
> /* Exception: if both the interface blocks are implicitly declared,
> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
> index de56945..b856631 100644
> --- a/src/compiler/glsl/linker.cpp
> +++ b/src/compiler/glsl/linker.cpp
> @@ -216,7 +216,7 @@ public:
> * array using an index too large for its actual size assigned at link
> * time.
> */
> - if (var->data.max_array_access >= this->num_vertices) {
> + if (var->data.max_array_access >= (int)this->num_vertices) {
> linker_error(this->prog, "geometry shader accesses element %i of "
> "%s, but only %i input vertices\n",
> var->data.max_array_access, var->name, this->num_vertices);
> @@ -924,7 +924,7 @@ validate_intrastage_arrays(struct gl_shader_program *prog,
> if ((var->type->fields.array == existing->type->fields.array) &&
> ((var->type->length == 0)|| (existing->type->length == 0))) {
> if (var->type->length != 0) {
> - if (var->type->length <= existing->data.max_array_access) {
> + if ((int)var->type->length <= existing->data.max_array_access) {
> linker_error(prog, "%s `%s' declared as type "
> "`%s' but outermost dimension has an index"
> " of `%i'\n",
> @@ -935,7 +935,7 @@ validate_intrastage_arrays(struct gl_shader_program *prog,
> existing->type = var->type;
> return true;
> } else if (existing->type->length != 0) {
> - if(existing->type->length <= var->data.max_array_access &&
> + if((int)existing->type->length <= var->data.max_array_access &&
> !existing->data.from_ssbo_unsized_array) {
> linker_error(prog, "%s `%s' declared as type "
> "`%s' but outermost dimension has an index"
> @@ -1593,7 +1593,7 @@ private:
> */
> static const glsl_type *
> resize_interface_members(const glsl_type *type,
> - const unsigned *max_ifc_array_access,
> + const int *max_ifc_array_access,
> bool is_ssbo)
> {
> unsigned num_fields = type->length;
> @@ -2399,10 +2399,10 @@ update_array_sizes(struct gl_shader_program *prog)
> * Subroutine uniforms are not removed.
> */
> if (var->is_in_buffer_block() || var->type->contains_atomic() ||
> - var->type->contains_subroutine())
> + var->type->contains_subroutine() || var->constant_initializer)
> continue;
>
> - unsigned int size = var->data.max_array_access;
> + int size = var->data.max_array_access;
> for (unsigned j = 0; j < MESA_SHADER_STAGES; j++) {
> if (prog->_LinkedShaders[j] == NULL)
> continue;
> @@ -2419,7 +2419,7 @@ update_array_sizes(struct gl_shader_program *prog)
> }
> }
>
> - if (size + 1 != var->type->length) {
> + if (size + 1 != (int)var->type->length) {
> /* If this is a built-in uniform (i.e., it's backed by some
> * fixed-function state), adjust the number of state slots to
> * match the new array size. The number of slots per array entry
> diff --git a/src/mesa/main/ff_fragment_shader.cpp b/src/mesa/main/ff_fragment_shader.cpp
> index b0ce8c4..26bf162 100644
> --- a/src/mesa/main/ff_fragment_shader.cpp
> +++ b/src/mesa/main/ff_fragment_shader.cpp
> @@ -517,7 +517,7 @@ get_current_attrib(texenv_fragment_program *p, GLuint attrib)
>
> current = p->shader->symbols->get_variable("gl_CurrentAttribFragMESA");
> assert(current);
> - current->data.max_array_access = MAX2(current->data.max_array_access, attrib);
> + current->data.max_array_access = MAX2(current->data.max_array_access, (int)attrib);
> val = new(p->mem_ctx) ir_dereference_variable(current);
> ir_rvalue *index = new(p->mem_ctx) ir_constant(attrib);
> return new(p->mem_ctx) ir_dereference_array(val, index);
> @@ -561,7 +561,7 @@ get_source(texenv_fragment_program *p,
> var = p->shader->symbols->get_variable("gl_TextureEnvColor");
> assert(var);
> deref = new(p->mem_ctx) ir_dereference_variable(var);
> - var->data.max_array_access = MAX2(var->data.max_array_access, unit);
> + var->data.max_array_access = MAX2(var->data.max_array_access, (int)unit);
> return new(p->mem_ctx) ir_dereference_array(deref,
> new(p->mem_ctx) ir_constant(unit));
>
> @@ -893,7 +893,7 @@ static void load_texture( texenv_fragment_program *p, GLuint unit )
> texcoord = new(p->mem_ctx) ir_dereference_variable(tc_array);
> ir_rvalue *index = new(p->mem_ctx) ir_constant(unit);
> texcoord = new(p->mem_ctx) ir_dereference_array(texcoord, index);
> - tc_array->data.max_array_access = MAX2(tc_array->data.max_array_access, unit);
> + tc_array->data.max_array_access = MAX2(tc_array->data.max_array_access, (int)unit);
> }
>
> if (!p->state->unit[unit].enabled) {
> --
> 2.5.5
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list