[Mesa-dev] [PATCH v2] i965/glsl: don't add unused aoa element to the program resource list

Timothy Arceri tarceri at itsqueeze.com
Tue Nov 13 02:28:11 UTC 2018


Sorry for not getting back sooner on this one.

I'm leaning towards a NAK on this one. This is just under 300 new lines 
of code to work around a possibly over strict piglit test. While the 
test is not wrong an implementation is also not required to optimise 
away these unused elements.

If this was actually compressing/optimising the arrays 300+ plus lines 
would probably be ok. But all this code just to hide the elements from 
the resource list seems excessive. I know I suggested this as a possible 
alternative fix to actually optimising the arrays but now that I've seen 
the result I'm not so sure this is a good idea. This is my opinion happy 
to see what others think.

On 21/9/18 12:03 am, asimiklit.work at gmail.com wrote:
> From: Andrii Simiklit <andrii.simiklit at globallogic.com>
> 
> It fixes a bit incorrectly implemented ARB_program_interface_query.
> If input aoa element is unused in shader program
> the 'glGetProgramResourceIndex' function shouldn't
> return a valid resource index for it according to:
> ARB_program_interface_query spec:
>      " For an active variable declared as an array of an aggregate data type
>          (structures or arrays), a separate entry will be generated for each
>          active array element"
> 
> v2: - Fixed case where the non-AoA elements were removed form
>      the program resource list.
>      The KHR-GL46.enhanced_layouts.varying_structure_locations
>      test is passed now
>      - Fixed case where a dereference index is not constant:
>      The proper array length stored in 'deref->array->type->length'
>      not in 'deref->type->length'
>      - Fixed the output AoA elements handling:
>      The list of active input AoA elements should not be used for
>      output AoA elements
>      - Fixed the function name from 'enumiramte_active_elements' to
>      'enumerate_active_elements'
>      - Fixed errors handling during active AoA elements enumeration
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92822
> Signed-off-by: Andrii Simiklit <andrii.simiklit at globallogic.com>
> ---
>   src/compiler/Makefile.sources                 |   4 +-
>   .../glsl/ir_collect_active_aoa_elements.cpp   | 155 ++++++++++++++++++
>   .../glsl/ir_collect_active_aoa_elements.h     |  52 ++++++
>   src/compiler/glsl/linker.cpp                  |  91 ++++++++--
>   src/compiler/glsl/meson.build                 |   2 +
>   5 files changed, 289 insertions(+), 15 deletions(-)
>   create mode 100644 src/compiler/glsl/ir_collect_active_aoa_elements.cpp
>   create mode 100644 src/compiler/glsl/ir_collect_active_aoa_elements.h
> 
> diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources
> index d3b0656483..a2ba3e37f1 100644
> --- a/src/compiler/Makefile.sources
> +++ b/src/compiler/Makefile.sources
> @@ -154,7 +154,9 @@ LIBGLSL_FILES = \
>   	glsl/serialize.cpp \
>   	glsl/serialize.h \
>   	glsl/string_to_uint_map.cpp \
> -	glsl/string_to_uint_map.h
> +	glsl/string_to_uint_map.h \
> +	glsl/ir_collect_active_aoa_elements.cpp \
> +	glsl/ir_collect_active_aoa_elements.h
>   
>   LIBGLSL_SHADER_CACHE_FILES = \
>   	glsl/shader_cache.cpp \
> diff --git a/src/compiler/glsl/ir_collect_active_aoa_elements.cpp b/src/compiler/glsl/ir_collect_active_aoa_elements.cpp
> new file mode 100644
> index 0000000000..df0a41ad3e
> --- /dev/null
> +++ b/src/compiler/glsl/ir_collect_active_aoa_elements.cpp
> @@ -0,0 +1,155 @@
> +/*
> + * Copyright © 2018 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include "ir_collect_active_aoa_elements.h"
> +#include "program.h"
> +#include "linker_util.h"
> +#include "util/set.h"
> +#include "util/u_dynarray.h"
> +
> +namespace
> +{
> +    /***
> +     * Helps to collect the names of used aoa elements
> +     * to the accessed_elements set
> +     ***/
> +    struct elem_inserter
> +    {
> +        elem_inserter(struct set *accessed_elements_)
> +        : accessed_elements(accessed_elements_)
> +        {}
> +        bool operator ()(const char *name)
> +        {
> +            bool oval = true;
> +            if (NULL == _mesa_set_search(accessed_elements, name)) {
> +                oval = (_mesa_set_add(accessed_elements, name) != NULL);
> +            }
> +            return oval;
> +        }
> +        struct set *accessed_elements;
> +    };
> +
> +    bool
> +    ir_is_array_deref(ir_rvalue *const ir)
> +    {
> +        return (ir_type_dereference_array == ir->ir_type);
> +    }
> +
> +    ir_variable *
> +    base_ir_dereference_var(ir_dereference_array *const ir)
> +    {
> +        ir_dereference_array const *base_ir = ir;
> +        while (ir_type_dereference_array == base_ir->array->ir_type) {
> +            base_ir = base_ir->array->as_dereference_array();
> +            assert(NULL != base_ir);
> +        }
> +
> +        ir_dereference_variable *const d =
> +            base_ir->array->as_dereference_variable();
> +        return (NULL == d) ? NULL : d->var;
> +    }
> +}
> +/**
> + * Helps to produce all combinations of used aoa elements
> + * for cases with constant and variable index.
> + **/
> +template <typename FunctorType>
> +inline bool enumerate_active_elements(void *memctx,
> +                                        ir_dereference_array **first,
> +                                        ir_dereference_array **item,
> +                                        const char *accessed_elem_name,
> +                                        FunctorType functor)
> +{
> +    if (NULL == accessed_elem_name) {
> +        return false;
> +    }
> +    if(item == (first - 1))
> +        return functor(accessed_elem_name);
> +
> +    ir_dereference_array *const deref = (*item);
> +    ir_constant const *const idx = deref->array_index->as_constant();
> +    if (NULL != idx) {
> +        const unsigned uidx = idx->get_uint_component(0U);
> +        char *elem = ralloc_asprintf(memctx, "%s[%u]",
> +                                    accessed_elem_name, uidx);
> +
> +        if(!enumerate_active_elements(memctx, first, item - 1, elem, functor))
> +            return false;
> +    }
> +    else {
> +        for (unsigned i = 0U; i < deref->array->type->length; ++i) {
> +            char *elem = ralloc_asprintf(memctx, "%s[%u]",
> +                                        accessed_elem_name, i);
> +            if(!enumerate_active_elements(memctx, first, item - 1, elem, functor))
> +                return false;
> +        }
> +    }
> +    return true;
> +}
> +
> +ir_visitor_status
> +ir_collect_active_aoa_elements::visit_enter(ir_dereference_array *ir)
> +{
> +    if (!ir_is_array_deref(ir->array))
> +        return visit_continue;
> +
> +    last_array_deref = ir;
> +    if (last_array_deref && last_array_deref->array == ir)
> +        return visit_continue;
> +
> +    ir_variable *const var = base_ir_dereference_var(ir);
> +    if (!var)
> +        return visit_continue;
> +
> +    struct util_dynarray indexes;
> +    util_dynarray_init(&indexes, NULL);
> +    /* The first element it is dereference
> +     * of regular array not aoa so skip it.
> +     * All the dereference which are stored in
> +     * the IR tree are storead as in the following instance:
> +     *
> +     * gl_Position = myarr[5][3][1];
> +     *
> +     * The first dereference will have an array_index =  [1]
> +     * The second dereference will have an array_index = [3]
> +     * The third dereference will have an array_index = [5]
> +     * So we need to traverse over the elements in the reverse direction
> +     * to enumerate all combinations of used aoa elements.
> +     */
> +    ir_rvalue *it = ir->array;
> +    while (ir_is_array_deref(it)) {
> +        ir_dereference_array *deref = it->as_dereference_array();
> +        assert(NULL != deref);
> +        assert(deref->array->type->is_array());
> +        util_dynarray_append(&indexes, ir_dereference_array*, deref);
> +        it = deref->array;
> +    }
> +    ir_dereference_array **last = util_dynarray_top_ptr(&indexes,
> +                                                    ir_dereference_array*);
> +    ir_dereference_array **first = util_dynarray_element(&indexes,
> +                                                    ir_dereference_array*, 0);
> +    error = !enumerate_active_elements(memctx, first, last, var->name,
> +                                     elem_inserter(accessed_elements));
> +    util_dynarray_fini(&indexes);
> +    return error ? visit_stop : visit_continue_with_parent;
> +}
> \ No newline at end of file
> diff --git a/src/compiler/glsl/ir_collect_active_aoa_elements.h b/src/compiler/glsl/ir_collect_active_aoa_elements.h
> new file mode 100644
> index 0000000000..5ca1dca7ac
> --- /dev/null
> +++ b/src/compiler/glsl/ir_collect_active_aoa_elements.h
> @@ -0,0 +1,52 @@
> +/*
> + * Copyright © 2018 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#ifndef IR_COLLECT_ACTIVE_AOA_ELEMENTS_H
> +#define IR_COLLECT_ACTIVE_AOA_ELEMENTS_H
> +
> +#include "ir.h"
> +#include "util/hash_table.h"
> +
> +class ir_collect_active_aoa_elements : public ir_hierarchical_visitor {
> +public:
> +   ir_collect_active_aoa_elements(void *memctx_,
> +                                struct set *accessed_elements_)
> +      : memctx(memctx_),
> +      accessed_elements(accessed_elements_),
> +      last_array_deref(0),
> +      error(false)
> +   {
> +   }
> +   virtual ir_visitor_status visit_enter(ir_dereference_array *);
> +   bool has_error() const { return this->error; }
> +private:
> +   void *memctx;
> +   struct set *accessed_elements;
> +   /**
> +    * Used to prevent some redundant calculations.
> +    */
> +   ir_dereference_array *last_array_deref;
> +   bool error;
> +};
> +
> +#endif /* IR_COLLECT_ACTIVE_AOA_ELEMENTS_H */
> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
> index 3fde7e78d3..b41cd735ae 100644
> --- a/src/compiler/glsl/linker.cpp
> +++ b/src/compiler/glsl/linker.cpp
> @@ -83,6 +83,7 @@
>   #include "ir_uniform.h"
>   #include "builtin_functions.h"
>   #include "shader_cache.h"
> +#include "ir_collect_active_aoa_elements.h"
>   #include "util/u_string.h"
>   #include "util/u_math.h"
>   
> @@ -94,6 +95,32 @@
>   
>   namespace {
>   
> +struct set * find_active_aoa_elements(void *mem_ctx,
> +                    gl_linked_shader * linked_shader,
> +                    struct gl_shader_program *shProg)
> +{
> +   struct set *active_aoa_elems =
> +                _mesa_set_create(mem_ctx, _mesa_key_hash_string,
> +                                 _mesa_key_string_equal);
> +   ir_collect_active_aoa_elements v(mem_ctx, active_aoa_elems);
> +   visit_list_elements(&v, linked_shader->ir);
> +#if 0
> +   if(!v.has_error()) {
> +      set_entry *entry;
> +      /* Print the active array of arrays "aoa" elements */
> +      set_foreach(active_aoa_elems, entry)
> +      {
> +          fprintf(stderr, "used aoa element : %s\n", (const char *)entry->key);
> +      }
> +   } else {
> +      fprintf(stderr, "error: ir_collect_active_aoa_elements completed with an error!\n");
> +   }
> +#endif
> +    if(v.has_error()) {
> +       linker_error(shProg, "error during active AoA elements enumeration!\n");
> +    }
> +    return v.has_error() ? NULL : active_aoa_elems;
> +}
>   struct find_variable {
>      const char *name;
>      bool found;
> @@ -3880,7 +3907,9 @@ add_shader_variable(const struct gl_context *ctx,
>                       const char *name, const glsl_type *type,
>                       bool use_implicit_location, int location,
>                       bool inouts_share_location,
> -                    const glsl_type *outermost_struct_type = NULL)
> +                    const glsl_type *outermost_struct_type = NULL,
> +                    struct set *active_aoa_elems = NULL,
> +                    uint32_t array_dimension = 0)
>   {
>      const glsl_type *interface_type = var->get_interface_type();
>   
> @@ -3942,7 +3971,8 @@ add_shader_variable(const struct gl_context *ctx,
>                                     stage_mask, programInterface,
>                                     var, field_name, field->type,
>                                     use_implicit_location, field_location,
> -                                  false, outermost_struct_type))
> +                                  false, outermost_struct_type,
> +                                  NULL, 0U))
>               return false;
>   
>            field_location += field->type->count_attribute_slots(false);
> @@ -3967,19 +3997,40 @@ add_shader_variable(const struct gl_context *ctx,
>          *      separate active variable."
>          */
>         const struct glsl_type *array_type = type->fields.array;
> -      if (array_type->base_type == GLSL_TYPE_STRUCT ||
> -          array_type->base_type == GLSL_TYPE_ARRAY) {
> +      if (array_type->is_record() || array_type->is_array()) {
> +         const bool is_aoa = array_type->is_array_of_arrays();
>            unsigned elem_location = location;
>            unsigned stride = inouts_share_location ? 0 :
>                              array_type->count_attribute_slots(false);
>            for (unsigned i = 0; i < type->length; i++) {
>               char *elem = ralloc_asprintf(shProg, "%s[%d]", name, i);
> -            if (!add_shader_variable(ctx, shProg, resource_set,
> -                                     stage_mask, programInterface,
> -                                     var, elem, array_type,
> -                                     use_implicit_location, elem_location,
> -                                     false, outermost_struct_type))
> -               return false;
> +            /* Single dimensional array of structs is accepted by default*/
> +            const bool acceptable_elem = (array_dimension < 1u  &&
> +                                            array_type->is_record()) ||
> +                    /*if next type has an aoa type
> +                    * the aoa elem name is not completed yet
> +                    * and we will just come here again
> +                    **/
> +                    is_aoa ||
> +                    /* if active_aoa_elems is NULL we will add all elements
> +                    */
> +                    (NULL == active_aoa_elems) ||
> +                    /* if this aoa element is used
> +                    * it can be added as resource
> +                    * otherwise shouldn't be added
> +                    **/
> +                    (NULL != _mesa_set_search(active_aoa_elems, elem));
> +            if(acceptable_elem) {
> +               if (!add_shader_variable(ctx, shProg, resource_set,
> +                               stage_mask, programInterface,
> +                               var, elem, array_type,
> +                               use_implicit_location, elem_location,
> +                               false, outermost_struct_type,
> +                               /*Pass the NULL to accept all elements*/
> +                               is_aoa ? active_aoa_elems : NULL,
> +                               array_dimension + 1U))
> +                    return false;
> +            }
>               elem_location += stride;
>            }
>            return true;
> @@ -4025,7 +4076,8 @@ static bool
>   add_interface_variables(const struct gl_context *ctx,
>                           struct gl_shader_program *shProg,
>                           struct set *resource_set,
> -                        unsigned stage, GLenum programInterface)
> +                        unsigned stage, GLenum programInterface,
> +                        struct set *active_aoa_elems)
>   {
>      exec_list *ir = shProg->_LinkedShaders[stage]->ir;
>   
> @@ -4078,7 +4130,8 @@ add_interface_variables(const struct gl_context *ctx,
>                                  1 << stage, programInterface,
>                                  var, var->name, var->type, vs_input_or_fs_output,
>                                  var->data.location - loc_bias,
> -                               inout_has_same_location(var, stage)))
> +                               inout_has_same_location(var, stage),
> +                               NULL, active_aoa_elems))
>            return false;
>      }
>      return true;
> @@ -4415,13 +4468,22 @@ build_program_resource_list(struct gl_context *ctx,
>      if (!add_fragdata_arrays(ctx, shProg, resource_set))
>         return;
>   
> +   // temporary mem context for program res building
> +   void *res_build_mem_ctx = ralloc_context(NULL);
> +   struct set *in_active_aoa_elems = find_active_aoa_elements(res_build_mem_ctx,
> +                                        shProg->_LinkedShaders[input_stage], shProg);
>      /* Add inputs and outputs to the resource list. */
>      if (!add_interface_variables(ctx, shProg, resource_set,
> -                                input_stage, GL_PROGRAM_INPUT))
> +                                input_stage, GL_PROGRAM_INPUT,
> +                                in_active_aoa_elems))
>         return;
>   
> +   struct set *out_active_aoa_elems = find_active_aoa_elements(res_build_mem_ctx,
> +                                        shProg->_LinkedShaders[output_stage], shProg);
> +
>      if (!add_interface_variables(ctx, shProg, resource_set,
> -                                output_stage, GL_PROGRAM_OUTPUT))
> +                                output_stage, GL_PROGRAM_OUTPUT,
> +                                out_active_aoa_elems))
>         return;
>   
>      if (shProg->last_vert_prog) {
> @@ -4538,6 +4600,7 @@ build_program_resource_list(struct gl_context *ctx,
>      }
>   
>      _mesa_set_destroy(resource_set, NULL);
> +   ralloc_free(res_build_mem_ctx);
>   }
>   
>   /**
> diff --git a/src/compiler/glsl/meson.build b/src/compiler/glsl/meson.build
> index 7e4dd2929a..fffae246af 100644
> --- a/src/compiler/glsl/meson.build
> +++ b/src/compiler/glsl/meson.build
> @@ -198,6 +198,8 @@ files_libglsl = files(
>     'serialize.h',
>     'shader_cache.cpp',
>     'shader_cache.h',
> +  'ir_collect_active_aoa_elements.cpp',
> +  'ir_collect_active_aoa_elements.h',
>   )
>   
>   files_libglsl_standalone = files(
> 


More information about the mesa-dev mailing list