[Mesa-dev] [PATCH] glsl/linker: remove ubo explicit binding handling at uniform_initializers
Timothy Arceri
tarceri at itsqueeze.com
Fri Feb 9 02:20:45 UTC 2018
On 08/02/18 18:34, Alejandro Piñeiro wrote:
> 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.
Looks good to me. Just process_array_leaf -> process_block_array_leaf
above, with that fixed.
Reviewed-by: Timothy Arceri <tarceri at itsqueeze.com>
>>
>> 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 {
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
More information about the mesa-dev
mailing list