[Mesa-dev] [PATCH v2] i965/glsl: don't add unused aoa element to the program resource list
Tapani Pälli
tapani.palli at intel.com
Thu Nov 15 08:28:46 UTC 2018
Hi;
On 11/13/18 4:28 AM, Timothy Arceri wrote:
> 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.
This is exactly how I feel as well. Functionally correct but way too
much code considering the issue :/ I believe we do detect elements that
get used during complication and linking so perhaps do that and in the
end modify resource list, remove (or mark dirty) all the unused ones?
> 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