[Mesa-dev] [PATCH 4/4] linker: Accurately track gl_uniform_block::stageref
Nicolai Hähnle
nhaehnle at gmail.com
Wed Nov 9 11:16:15 UTC 2016
On 08.11.2016 06:50, Ian Romanick wrote:
> From: Ian Romanick <ian.d.romanick at intel.com>
>
> 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 including
> ESEXT-CTS.geometry_shader.program_resource.program_resource. However,
> it makes quite a few more fail. 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.
Thank you for tackling this -- I've just tried to ignore it for now.
FWIW, I expect that this also affects the GL CTS in a similar way.
Eliminating the assignment ought to be perfectly fine. The only
exception I can think of would be if the variable (or block) had a
volatile qualifier, because the GLSL spec says that "when a volatile
variable is written, its value must be written to the underlying
memory". None of the other memory types have similar language.
So I'd suggest changing the test to include a volatile qualifier, and
possibly fixing the optimization we're doing to honor that. But it would
be good to clarify with Khronos.
With or without Ilia's suggestion, the patches are
Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>
>
> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> ---
> 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 d70614f..29cd24c 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 aceea8d..693a50b 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];
> }
> }
>
More information about the mesa-dev
mailing list