<div dir="ltr"><div dir="ltr"><div dir="ltr">Hello, Thanks a lot for reviewing and testing on CI. I will send new patch as soon as fix all these issues. Regards, Andrii.</div></div></div> <div class="gmail_quote"><div dir="ltr">On Thu, Sep 13, 2018 at 2:32 PM Tapani Pälli <<a href="mailto:tapani.palli@intel.com">tapani.palli@intel.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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 <<a href="mailto:asimiklit.work@gmail.com" target="_blank">asimiklit.work@gmail.com</a> > <mailto:<a href="mailto:asimiklit.work@gmail.com" target="_blank">asimiklit.work@gmail.com</a>>> wrote: > > From: Andrii Simiklit <<a href="mailto:andrii.simiklit@globallogic.com" target="_blank">andrii.simiklit@globallogic.com</a> > <mailto:<a href="mailto:andrii.simiklit@globallogic.com" target="_blank">andrii.simiklit@globallogic.com</a>>> > > 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: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=92822" rel="noreferrer" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=92822</a> > Signed-off-by: Andrii Simiklit <<a href="mailto:andrii.simiklit@globallogic.com" target="_blank">andrii.simiklit@globallogic.com</a> > <mailto:<a href="mailto:andrii.simiklit@globallogic.com" target="_blank">andrii.simiklit@globallogic.com</a>>> > --- > 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 > </blockquote></div>