[Mesa-dev] [PATCH] i965/glsl: don't add unused aoa elements to the program resource list
andrey simiklit
asimiklit.work at gmail.com
Fri Sep 14 07:13:35 UTC 2018
Hello,
Thanks a lot for reviewing and testing on CI.
I will send new patch as soon as fix all these issues.
Regards,
Andrii.
On Thu, Sep 13, 2018 at 2:32 PM Tapani Pälli <tapani.palli at intel.com> wrote:
>
>
> On 09/07/2018 03:25 PM, andrey simiklit wrote:
> > Hi all,
> >
> > Could somebody run it on CI to confirm that this patch fixes one test
> > and not add regressions or maybe even take a look at this patch)
>
> CI reports failures in following conformance test:
>
> KHR-GL46.enhanced_layouts.varying_structure_locations
>
> As example:
>
> --- 8< ---
> Test case (float) failed.
> FAILURE. Test case: mat2. Inspection of separable draw program interface
> failed:
> Failed to query program for varying: single. Reason:
> GetProgramResourceiv: glGetError() returned GL_INVALID_VALUE at
> gl4cEnhancedLayoutsTests.cpp:3073
> Failed to query program for varying: array. Reason:
> GetProgramResourceiv: glGetError() returned GL_INVALID_VALUE at
> gl4cEnhancedLayoutsTests.cpp:3073
> --- 8< ---
>
> There are similar/same failures for each glsl type.
>
> Please find couple of small review notes below.
>
> > Regards,
> > Andrii.
> >
> > On Fri, Aug 31, 2018 at 5:13 PM <asimiklit.work at gmail.com
> > <mailto:asimiklit.work at gmail.com>> wrote:
> >
> > From: Andrii Simiklit <andrii.simiklit at globallogic.com
> > <mailto: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"
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92822
> > Signed-off-by: Andrii Simiklit <andrii.simiklit at globallogic.com
> > <mailto:andrii.simiklit at globallogic.com>>
> > ---
> > src/compiler/Makefile.sources | 4 +-
> > .../glsl/ir_collect_active_aoa_elements.cpp | 148
> > +++++++++++++++++++++
> > src/compiler/glsl/ir_collect_active_aoa_elements.h | 49 +++++++
> > src/compiler/glsl/linker.cpp | 75
> +++++++++--
> > src/compiler/glsl/meson.build | 2 +
> > 5 files changed, 265 insertions(+), 13 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 d3b0656..a2ba3e3 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 0000000..50042c7
> > --- /dev/null
> > +++ b/src/compiler/glsl/ir_collect_active_aoa_elements.cpp
> > @@ -0,0 +1,148 @@
> > +/*
> > + * 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_)
> > + {}
> > + void operator ()(const char *name)
> > + {
> > + if (NULL == _mesa_set_search(accessed_elements, name)) {
> > + _mesa_set_add(accessed_elements, name);
> > + }
> > + }
> > + 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 void enumiramte_active_elements(void *memctx,
>
> enumerate
>
> > + ir_dereference_array
> **first,
> > + ir_dereference_array **item,
> > + const char
> *accessed_elem_name,
> > + FunctorType functor)
> > +{
> > + if(item == (first - 1)) {
> > + functor(accessed_elem_name);
> > + return;
> > + }
> > +
> > + 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);
> > + enumiramte_active_elements(memctx, first, item - 1, elem,
> > functor);
> > + }
> > + else {
> > + for (unsigned i = 0U; i < deref->type->length; ++i) {
> > + char *elem = ralloc_asprintf(memctx, "%s[%u]",
> > + accessed_elem_name, i);
> > + enumiramte_active_elements(memctx, first, item - 1,
> > elem, functor);
> > + }
> > + }
> > +}
> > +
> > +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);
> > + enumiramte_active_elements(memctx, first, last, var->name,
> > + elem_inserter(accessed_elements));
> > + util_dynarray_fini(&indexes);
> > + return 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 0000000..fb8a8cd
> > --- /dev/null
> > +++ b/src/compiler/glsl/ir_collect_active_aoa_elements.h
> > @@ -0,0 +1,49 @@
> > +/*
> > + * 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)
> > + {
> > + }
> > + virtual ir_visitor_status visit_enter(ir_dereference_array *);
> > +private:
> > + void *memctx;
> > + struct set *accessed_elements;
> > + /**
> > + * Used to prevent some redundant calculations.
> > + */
> > + ir_dereference_array *last_array_deref;
> > +};
> > +
> > +#endif /* IR_COLLECT_ACTIVE_AOA_ELEMENTS_H */
> > diff --git a/src/compiler/glsl/linker.cpp
> b/src/compiler/glsl/linker.cpp
> > index f08971d..8159404 100644
> > --- a/src/compiler/glsl/linker.cpp
> > +++ b/src/compiler/glsl/linker.cpp
> > @@ -84,6 +84,7 @@
> > #include "builtin_functions.h"
> > #include "shader_cache.h"
> > #include "util/u_string.h"
> > +#include "ir_collect_active_aoa_elements.h"
> >
> > #include "main/imports.h"
> > #include "main/shaderobj.h"
> > @@ -93,6 +94,24 @@
> >
> > namespace {
> >
> > +struct set * find_active_aoa_elements(void *mem_ctx,
> > + gl_linked_shader * linked_shader)
> > +{
> > + 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
> > + 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);
> > + }
> > +#endif
> > + return active_aoa_elems;
> > +}
> > struct find_variable {
> > const char *name;
> > bool found;
> > @@ -3819,7 +3838,8 @@ 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)
> > {
> > const glsl_type *interface_type = var->get_interface_type();
> >
> > @@ -3881,7 +3901,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))
> > return false;
> >
> > field_location +=
> field->type->count_attribute_slots(false);
> > @@ -3908,17 +3929,36 @@ add_shader_variable(const struct gl_context
> > *ctx,
> > const struct glsl_type *array_type = type->fields.array;
> > if (array_type->base_type == GLSL_TYPE_STRUCT ||
> > array_type->base_type == GLSL_TYPE_ARRAY) {
> > + const bool is_aoa = (array_type->fields.array->base_type ==
> > + GLSL_TYPE_ARRAY);
>
>
> I think this should rather use "array_type->is_array_of_arrays()".
>
> Maybe this block here is related to the failures (?) I have a feeling
> that we are handling some resources as aoa resources that are not really
> aoa resources.
>
>
> > 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;
> > + /*if next type has an array type
> > + * the aoa elem name is not completed yet
> > + * and we will just come here again
> > + **/
> > + const bool acceptable = is_aoa ||
> > + /* if active_aoa_elems is NULL it will work
> > as was */
> > + !active_aoa_elems ||
> > + /* if this aoa element is used
> > + * it can be added as resource
> > + * otherwise shouldn't be added
> > + **/
> > + _mesa_set_search(active_aoa_elems, elem) !=
> > NULL;
> > +
> > + if(acceptable)
> > + {
> > + if (!add_shader_variable(ctx, shProg, resource_set,
> > + stage_mask, programInterface,
> > + var, elem, array_type,
> > + use_implicit_location,
> > elem_location,
> > + false, outermost_struct_type,
> > + active_aoa_elems))
> > + return false;
> > + }
> > elem_location += stride;
> > }
> > return true;
> > @@ -3964,7 +4004,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;
> >
> > @@ -4017,7 +4058,9 @@ 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;
> > @@ -4354,15 +4397,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 *active_aoa_elems =
> > find_active_aoa_elements(res_build_mem_ctx,
> > +
> > shProg->_LinkedShaders[input_stage]);
> > /* 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,
> > + active_aoa_elems))
> > return;
> >
> > if (!add_interface_variables(ctx, shProg, resource_set,
> > - output_stage, GL_PROGRAM_OUTPUT))
> > + output_stage, GL_PROGRAM_OUTPUT,
> > + active_aoa_elems))
> > return;
> >
> > +
> > if (shProg->last_vert_prog) {
> > struct gl_transform_feedback_info *linked_xfb =
> > shProg->last_vert_prog->sh.LinkedTransformFeedback;
> > @@ -4477,6 +4527,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 7e4dd29..fffae24 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(
> > --
> > 2.7.4
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180914/58ebf1fc/attachment-0001.html>
More information about the mesa-dev
mailing list