<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div>Hi all,</div><div> </div><div>I have tested it on Intel CI and it just fixes test without any regression:</div><div><a href="https://mesa-ci.01.org/global_logic/builds/27/group/63a9f0ea7bb98050796b649e85481845">https://mesa-ci.01.org/global_logic/builds/27/group/63a9f0ea7bb98050796b649e85481845</a></div> <div>This patch fixes piglit test:</div><div> <a href="https://bugs.freedesktop.org/show_bug.cgi?id=92822">https://bugs.freedesktop.org/show_bug.cgi?id=92822</a></div><div dir="ltr"> At the moment it is marked as expected failure.</div><div><div dir="ltr">So it fails with message: "ERROR: This test passed when it expected failure"</div></div><div> </div><div>It would be grate if somebody reviews this patch) </div><div> </div><div>Thanks,</div><div>Andrii.</div><div><div class="gmail_quote"><div dir="ltr">On Thu, Sep 20, 2018 at 5:03 PM <<a href="mailto:asimiklit.work@gmail.com">asimiklit.work@gmail.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">From: Andrii Simiklit <<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" 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: <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>> --- 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( -- 2.17.1 </blockquote></div></div></div></div></div></div>