[Mesa-dev] [PATCH] glsl: fix lowering of UBO references of named blocks

[Nicolai Hähnle]

On 02.11.2016 15:46, Iago Toral wrote:
> On Mon, 2016-10-31 at 22:19 +0100, Nicolai Hähnle wrote:
>> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>>
>> When a UBO reference has the form block_name.foo where block_name
>> refers
>> to a block where the first member has a non-zero offset, the base
>> offset
>> was incorrectly added to the reference.
>
> The patch looks good to me and is:
>
> Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>

Thanks!


> I am curious: in what scenario doesn't the first member of a block have
> a zero offset?

It's an ARB_enhanced_layouts thing. As the commit message says, I ran 
across this in the GL CTS because it hit an assertion in radeonsi (we 
miscompiled it, but the GL CTS didn't actually check for correct 
execution...). I've sent a Piglit patch that tests this case as well.

Cheers,
Nicolai


>
> Iago
>
>> Fixes an assertion triggered in debug builds by
>> GL45-CTS.enhanced_layouts.uniform_block_layout_qualifier_conflict.
>> That test
>> doesn't properly check for correct execution in this case, so I am
>> also
>> going to send out a piglit test.
>>
>> Cc: 13.0 <mesa-stable at lists.freedesktop.org>
>> ---
>>  src/compiler/glsl/lower_ubo_reference.cpp | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/compiler/glsl/lower_ubo_reference.cpp
>> b/src/compiler/glsl/lower_ubo_reference.cpp
>> index 37134a9..eafa1dd 100644
>> --- a/src/compiler/glsl/lower_ubo_reference.cpp
>> +++ b/src/compiler/glsl/lower_ubo_reference.cpp
>> @@ -100,21 +100,20 @@ public:
>>
>>     unsigned calculate_unsized_array_stride(ir_dereference *deref,
>>                                             enum
>> glsl_interface_packing packing);
>>
>>     ir_call *lower_ssbo_atomic_intrinsic(ir_call *ir);
>>     ir_call *check_for_ssbo_atomic_intrinsic(ir_call *ir);
>>     ir_visitor_status visit_enter(ir_call *ir);
>>
>>     struct gl_linked_shader *shader;
>>     bool clamp_block_indices;
>> -   struct gl_uniform_buffer_variable *ubo_var;
>>     const struct glsl_struct_field *struct_field;
>>     ir_variable *variable;
>>     ir_rvalue *uniform_block;
>>     bool progress;
>>  };
>>
>>  /**
>>   * Determine the name of the interface block field
>>   *
>>   * This is the name of the specific member as it would appear in the
>> @@ -301,31 +300,32 @@
>> lower_ubo_reference_visitor::setup_for_load_or_store(void *mem_ctx,
>>        if (strcmp(field_name, blocks[i]->Name) == 0) {
>>
>>           ir_constant *index = new(mem_ctx) ir_constant(i);
>>
>>           if (nonconst_block_index) {
>>              this->uniform_block = add(nonconst_block_index, index);
>>           } else {
>>              this->uniform_block = index;
>>           }
>>
>> -         this->ubo_var = var->is_interface_instance()
>> -            ? &blocks[i]->Uniforms[0] : &blocks[i]->Uniforms[var-
>>> data.location];
>> +         if (var->is_interface_instance()) {
>> +            *const_offset = 0;
>> +         } else {
>> +            *const_offset = blocks[i]->Uniforms[var-
>>> data.location].Offset;
>> +         }
>>
>>           break;
>>        }
>>     }
>>
>>     assert(this->uniform_block);
>>
>> -   *const_offset = ubo_var->Offset;
>> -
>>     this->struct_field = NULL;
>>     setup_buffer_access(mem_ctx, deref, offset, const_offset,
>> row_major,
>>                         matrix_columns, &this->struct_field,
>> packing);
>>  }
>>
>>  void
>>  lower_ubo_reference_visitor::handle_rvalue(ir_rvalue **rvalue)
>>  {
>>     if (!*rvalue)
>>        return;