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.pa...@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.w...@gmail.com > > <mailto:asimiklit.w...@gmail.com>> wrote: > > > > From: Andrii Simiklit <andrii.simik...@globallogic.com > > <mailto:andrii.simik...@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.simik...@globallogic.com > > <mailto:andrii.simik...@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 > > >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
