From 084105c213dbf4c309453b33a966aac6ae9242f3 Mon Sep 17 00:00:00 2001 From: Ian Romanick Date: Mon, 7 Nov 2016 15:59:35 -0800 Subject: [PATCH] linker: Accurately track gl_uniform_block::stageref MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit As the linked per-stage shaders are processed, mark any block that has a field that is accessed as referenced. When combining all the linked shaders, combine the per-stage stageref masks. This fixes a number of GLES CTS tests: ES31-CTS.core.geometry_shader.program_resource.program_resource ES32-CTS.core.geometry_shader.program_resource.program_resource ESEXT-CTS.geometry_shader.program_resource.program_resource piglit.gl45-cts.geometry_shader.program_resource.program_resource However, it makes quite a few more fail: ES31-CTS.functional.program_interface_query.buffer_variable.random.6 ES31-CTS.functional.program_interface_query.buffer_variable.referenced_by.compute.unnamed_block.float ES31-CTS.functional.program_interface_query.buffer_variable.referenced_by.separable_fragment.unnamed_block.float ES31-CTS.functional.program_interface_query.buffer_variable.referenced_by.vertex_fragment_only_fragment.unnamed_block.float ES31-CTS.functional.program_interface_query.buffer_variable.referenced_by.vertex_fragment.unnamed_block.float ES31-CTS.functional.program_interface_query.buffer_variable.referenced_by.vertex_geo_fragment_only_fragment.unnamed_block.float ES31-CTS.functional.program_interface_query.buffer_variable.referenced_by.vertex_geo_fragment.unnamed_block.float ES31-CTS.functional.program_interface_query.buffer_variable.referenced_by.vertex_tess_fragment_only_fragment.unnamed_block.float ES31-CTS.functional.program_interface_query.buffer_variable.referenced_by.vertex_tess_fragment.unnamed_block.float ES31-CTS.functional.program_interface_query.buffer_variable.referenced_by.vertex_tess_geo_fragment_only_fragment.unnamed_block.float ES31-CTS.functional.program_interface_query.buffer_variable.referenced_by.vertex_tess_geo_fragment.unnamed_block.float ES32-CTS.functional.program_interface_query.buffer_variable.random.6 ES32-CTS.functional.program_interface_query.buffer_variable.referenced_by.compute.unnamed_block.float ES32-CTS.functional.program_interface_query.buffer_variable.referenced_by.separable_fragment.unnamed_block.float ES32-CTS.functional.program_interface_query.buffer_variable.referenced_by.vertex_fragment_only_fragment.unnamed_block.float ES32-CTS.functional.program_interface_query.buffer_variable.referenced_by.vertex_fragment.unnamed_block.float ES32-CTS.functional.program_interface_query.buffer_variable.referenced_by.vertex_geo_fragment_only_fragment.unnamed_block.float ES32-CTS.functional.program_interface_query.buffer_variable.referenced_by.vertex_geo_fragment.unnamed_block.float ES32-CTS.functional.program_interface_query.buffer_variable.referenced_by.vertex_tess_fragment_only_fragment.unnamed_block.float ES32-CTS.functional.program_interface_query.buffer_variable.referenced_by.vertex_tess_fragment.unnamed_block.float ES32-CTS.functional.program_interface_query.buffer_variable.referenced_by.vertex_tess_geo_fragment_only_fragment.unnamed_block.float ES32-CTS.functional.program_interface_query.buffer_variable.referenced_by.vertex_tess_geo_fragment.unnamed_block.float I have diagnosed the failures, but I'm not sure whether we or the tests are wrong. After optimizations are applied, all of the tests are of the form: buffer X { float f; } x; void main() { x.f = x.f; } The test then queries that x is referenced by that shader stage. We eliminate the assignment of x.f to itself, and that removes the last reference to x. We report that x is not referenced, and the test fails. I do not know whether or not we are allowed to eliminate that assignment of x.f to itself. After discussions with the OpenGL ES group in Khronos, we believe that Mesa's behavior is correct. I will provide patches to the CTS tests to Khronos. Signed-off-by: Ian Romanick Reviewed-by: Nicolai Hähnle Reviewed-by: Kenneth Graunke --- src/compiler/glsl/link_uniforms.cpp | 65 +++++++++++++++++++++++++---- src/compiler/glsl/linker.cpp | 3 +- 2 files changed, 59 insertions(+), 9 deletions(-) diff --git a/src/compiler/glsl/link_uniforms.cpp b/src/compiler/glsl/link_uniforms.cpp index d3a77d58c3b..54adbdf38ff 100644 --- a/src/compiler/glsl/link_uniforms.cpp +++ b/src/compiler/glsl/link_uniforms.cpp @@ -28,6 +28,7 @@ #include "glsl_symbol_table.h" #include "program.h" #include "util/string_to_uint_map.h" +#include "ir_variable_refcount.h" /** * \file link_uniforms.cpp @@ -877,6 +878,15 @@ public: unsigned shader_shadow_samplers; }; +static bool +variable_is_referenced(ir_variable_refcount_visitor &v, ir_variable *var) +{ + ir_variable_refcount_entry *const entry = v.get_variable_entry(var); + + return entry->referenced_count > 0; + +} + /** * Walks the IR and update the references to uniform blocks in the * ir_variables to point at linked shader's list (previously, they @@ -884,8 +894,13 @@ public: * shaders). */ static void -link_update_uniform_buffer_variables(struct gl_linked_shader *shader) +link_update_uniform_buffer_variables(struct gl_linked_shader *shader, + unsigned stage) { + ir_variable_refcount_visitor v; + + v.run(shader->ir); + foreach_in_list(ir_instruction, node, shader->ir) { ir_variable *const var = node->as_variable(); @@ -895,7 +910,44 @@ link_update_uniform_buffer_variables(struct gl_linked_shader *shader) assert(var->data.mode == ir_var_uniform || var->data.mode == ir_var_shader_storage); + unsigned num_blocks = var->data.mode == ir_var_uniform ? + shader->NumUniformBlocks : shader->NumShaderStorageBlocks; + struct gl_uniform_block **blks = var->data.mode == ir_var_uniform ? + shader->UniformBlocks : shader->ShaderStorageBlocks; + if (var->is_interface_instance()) { + if (variable_is_referenced(v, var)) { + /* Since this is an interface instance, the instance type will be + * same as the array-stripped variable type. If the variable type + * is an array, then the block names will be suffixed with [0] + * through [n-1]. Unlike for non-interface instances, there will + * not be structure types here, so the only name sentinel that we + * have to worry about is [. + */ + assert(var->type->without_array() == var->get_interface_type()); + const char sentinel = var->type->is_array() ? '[' : '\0'; + + const ptrdiff_t len = strlen(var->get_interface_type()->name); + for (unsigned i = 0; i < num_blocks; i++) { + const char *const begin = blks[i]->Name; + const char *const end = strchr(begin, sentinel); + + if (end == NULL) + continue; + + if (len != (end - begin)) + continue; + + /* Even when a match is found, do not "break" here. This could + * be an array of instances, and all elements of the array need + * to be marked as referenced. + */ + if (strncmp(begin, var->get_interface_type()->name, len) == 0) { + blks[i]->stageref |= 1U << stage; + } + } + } + var->data.location = 0; continue; } @@ -910,11 +962,6 @@ link_update_uniform_buffer_variables(struct gl_linked_shader *shader) sentinel = '['; } - unsigned num_blocks = var->data.mode == ir_var_uniform ? - shader->NumUniformBlocks : shader->NumShaderStorageBlocks; - struct gl_uniform_block **blks = var->data.mode == ir_var_uniform ? - shader->UniformBlocks : shader->ShaderStorageBlocks; - const unsigned l = strlen(var->name); for (unsigned i = 0; i < num_blocks; i++) { for (unsigned j = 0; j < blks[i]->NumUniforms; j++) { @@ -935,6 +982,10 @@ link_update_uniform_buffer_variables(struct gl_linked_shader *shader) if (found) { var->data.location = j; + + if (variable_is_referenced(v, var)) + blks[i]->stageref |= 1U << stage; + break; } } @@ -1257,7 +1308,7 @@ link_assign_uniform_locations(struct gl_shader_program *prog, memset(sh->SamplerUnits, 0, sizeof(sh->SamplerUnits)); memset(sh->ImageUnits, 0, sizeof(sh->ImageUnits)); - link_update_uniform_buffer_variables(sh); + link_update_uniform_buffer_variables(sh, i); /* Reset various per-shader target counts. */ diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp index aceea8d520e..693a50b20d5 100644 --- a/src/compiler/glsl/linker.cpp +++ b/src/compiler/glsl/linker.cpp @@ -1191,11 +1191,10 @@ interstage_cross_validate_uniform_blocks(struct gl_shader_program *prog, if (stage_index != -1) { struct gl_linked_shader *sh = prog->_LinkedShaders[i]; - blks[j].stageref |= (1 << i); - struct gl_uniform_block **sh_blks = validate_ssbo ? sh->ShaderStorageBlocks : sh->UniformBlocks; + blks[j].stageref |= sh_blks[stage_index]->stageref; sh_blks[stage_index] = &blks[j]; } } -- 2.30.2