[Mesa-dev] [PATCH] glsl/linker: remove ubo explicit binding handling at uniform_initializers

[Alejandro Piñeiro]

Ping.

FWIW, when I sent this patch, I also sent a test to the piglit ml, that
got reviewed and pushed:
https://cgit.freedesktop.org/piglit/commit/?id=88a9a99794f6108b656d4142cecd373b7995863c

To run it:
./bin/shader_runner
tests/spec/arb_arrays_of_arrays/execution/ubo/fs-const-explicit-binding.shader_test
-auto -fbo

right now, that test is crashing (at least on i965 driver, but it should
crash in most drivers).


On 24/01/18 15:30, Alejandro Piñeiro wrote:
> This is already handled at link_uniform_blocks, specifically at
> process_array_leaf.
>
> Additionally, this code was not handling correctly arrays of
> arrays. When creating the name of the block to set the binding, it
> only took into account the first level, so any attempt to set a
> explicit binding on a array of array ubo would trigger an assertion.
> ---
>
> I have just send a new test to the piglit list ("arb_arrays_of_arrays:
> add ubo test with explicit binding") that crashes without this
> patch. Without this patch it reachs the "unreachable("")" at the
> method set_block_binding that is being removed.
>
> It reaches that point because set_block_binding is called with a wrong
> block_name. When it is called with an array, it uses only the first
> layer, so for arrays of arrays, it creates a wrong name (block[0]
> instead of block[0][0] for example). See below, after the "Section
> 4.4.3" comment.
>
> Initially I was trying to solve this problem by fixing how the block
> name is created, until I realized that the binding at that point was
> already correct. Then I took a look to the code, and found that
> link_uniform_blocks were already handling setting the explicit
> binding, at process_block_array_leaf. Skimming it, seemed to handle
> all the corner cases. I also sent this patch to Intel CI and I didn't
> get any regression. And in any case, if there is any case not covered
> by the tests, I think that it would be better to handle explicit
> binding for ubos in just one place.
>
>
>  src/compiler/glsl/link_uniform_initializers.cpp | 58 +------------------------
>  1 file changed, 2 insertions(+), 56 deletions(-)
>
> diff --git a/src/compiler/glsl/link_uniform_initializers.cpp b/src/compiler/glsl/link_uniform_initializers.cpp
> index 97796e721bf..35522f76467 100644
> --- a/src/compiler/glsl/link_uniform_initializers.cpp
> +++ b/src/compiler/glsl/link_uniform_initializers.cpp
> @@ -182,26 +182,6 @@ set_opaque_binding(void *mem_ctx, gl_shader_program *prog,
>     }
>  }
>  
> -static void
> -set_block_binding(gl_shader_program *prog, const char *block_name,
> -                  unsigned mode, int binding)
> -{
> -   unsigned num_blocks = mode == ir_var_uniform ?
> -      prog->data->NumUniformBlocks :
> -      prog->data->NumShaderStorageBlocks;
> -   struct gl_uniform_block *blks = mode == ir_var_uniform ?
> -      prog->data->UniformBlocks : prog->data->ShaderStorageBlocks;
> -
> -   for (unsigned i = 0; i < num_blocks; i++) {
> -      if (!strcmp(blks[i].Name, block_name)) {
> -         blks[i].Binding = binding;
> -         return;
> -      }
> -   }
> -
> -   unreachable("Failed to initialize block binding");
> -}
> -
>  void
>  set_uniform_initializer(void *mem_ctx, gl_shader_program *prog,
>                          const char *name, const glsl_type *type,
> @@ -307,43 +287,9 @@ link_set_uniform_initializers(struct gl_shader_program *prog,
>                 linker::set_opaque_binding(mem_ctx, prog, var, var->type,
>                                            var->name, &binding);
>              } else if (var->is_in_buffer_block()) {
> -               const glsl_type *const iface_type = var->get_interface_type();
> -
> -               /* If the variable is an array and it is an interface instance,
> -                * we need to set the binding for each array element.  Just
> -                * checking that the variable is an array is not sufficient.
> -                * The variable could be an array element of a uniform block
> -                * that lacks an instance name.  For example:
> -                *
> -                *     uniform U {
> -                *         float f[4];
> -                *     };
> -                *
> -                * In this case "f" would pass is_in_buffer_block (above) and
> -                * type->is_array(), but it will fail is_interface_instance().
> +               /* This case is handled by link_uniform_blocks (at
> +                * process_block_array_leaf)
>                  */
> -               if (var->is_interface_instance() && var->type->is_array()) {
> -                  for (unsigned i = 0; i < var->type->length; i++) {
> -                     const char *name =
> -                        ralloc_asprintf(mem_ctx, "%s[%u]", iface_type->name, i);
> -
> -                     /* Section 4.4.3 (Uniform Block Layout Qualifiers) of the
> -                      * GLSL 4.20 spec says:
> -                      *
> -                      *     "If the binding identifier is used with a uniform
> -                      *     block instanced as an array then the first element
> -                      *     of the array takes the specified block binding and
> -                      *     each subsequent element takes the next consecutive
> -                      *     uniform block binding point."
> -                      */
> -                     linker::set_block_binding(prog, name, var->data.mode,
> -                                               var->data.binding + i);
> -                  }
> -               } else {
> -                  linker::set_block_binding(prog, iface_type->name,
> -                                            var->data.mode,
> -                                            var->data.binding);
> -               }
>              } else if (type->contains_atomic()) {
>                 /* we don't actually need to do anything. */
>              } else {