[Mesa-dev] [PATCH v3 5/6] glsl: fix matrix stride calculation for std430's row_major matrices with two columns

[Samuel Iglesias Gonsálvez]

On 08/10/15 00:47, Timothy Arceri wrote:
> On Wed, 2015-10-07 at 09:21 +0200, Samuel Iglesias Gonsalvez wrote:
>> It doesn't round up to vec4 size.
>>
>> Fixes 15 dEQP tests:
>>
>> dEQP
>> -GLES31.functional.ssbo.layout.single_basic_type.std430.row_major_low
>> p_mat2
>> dEQP
>> -GLES31.functional.ssbo.layout.single_basic_type.std430.row_major_med
>> iump_mat2
>> dEQP
>> -GLES31.functional.ssbo.layout.single_basic_type.std430.row_major_hig
>> hp_mat2
>> dEQP
>> -GLES31.functional.ssbo.layout.single_basic_type.std430.row_major_low
>> p_mat2x3
>> dEQP
>> -GLES31.functional.ssbo.layout.single_basic_type.std430.row_major_med
>> iump_mat2x3
>> dEQP
>> -GLES31.functional.ssbo.layout.single_basic_type.std430.row_major_hig
>> hp_mat2x3
>> dEQP
>> -GLES31.functional.ssbo.layout.single_basic_type.std430.row_major_low
>> p_mat2x4
>> dEQP
>> -GLES31.functional.ssbo.layout.single_basic_type.std430.row_major_med
>> iump_mat2x4
>> dEQP
>> -GLES31.functional.ssbo.layout.single_basic_type.std430.row_major_hig
>> hp_mat2x4
>> dEQP
>> -GLES31.functional.ssbo.layout.single_basic_array.std430.row_major_ma
>> t2
>> dEQP
>> -GLES31.functional.ssbo.layout.single_basic_array.std430.row_major_ma
>> t2x3
>> dEQP
>> -GLES31.functional.ssbo.layout.single_basic_array.std430.row_major_ma
>> t2x4
>> dEQP
>> -GLES31.functional.ssbo.layout.instance_array_basic_type.std430.row_m
>> ajor_mat2
>> dEQP
>> -GLES31.functional.ssbo.layout.instance_array_basic_type.std430.row_m
>> ajor_mat2x3
>> dEQP
>> -GLES31.functional.ssbo.layout.instance_array_basic_type.std430.row_m
>> ajor_mat2x4
>>
>> Signed-off-by: Samuel Iglesias Gonsalvez <siglesias at igalia.com>
>> ---
>>  src/glsl/lower_ubo_reference.cpp | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/glsl/lower_ubo_reference.cpp
>> b/src/glsl/lower_ubo_reference.cpp
>> index 247620e..183435e 100644
>> --- a/src/glsl/lower_ubo_reference.cpp
>> +++ b/src/glsl/lower_ubo_reference.cpp
>> @@ -744,7 +744,14 @@ lower_ubo_reference_visitor::emit_access(bool
>> is_write,
>>         * or 32 depending on the number of columns.
>>         */
>>        assert(matrix_columns <= 4);
>> -      unsigned matrix_stride = glsl_align(matrix_columns * N, 16);
>> +      unsigned matrix_stride = 0;
>> +      /* matrix stride for std430 mat2xY matrices are not rounded up
>> to
>> +       * vec4 size.
>> +       */
> 
> Is there a spec quote you can use here with section number spec version
> etc?
> 

Actually, this is the result of applying several rules:

>From OpenGL 4.3 spec, section 7.6.2.2 "Standard Uniform Block Layout":

"2. If the member is a two- or four-component vector with components
consuming N basic machine units, the base alignment is 2N or 4N,
respectively."
[...]
"4. If the member is an array of scalars or vectors, the base alignment
and array stride are set to match the base alignment of a single array
element, according to rules (1), (2), and (3), and rounded up to the
base alignment of a vec4."
[...]
"7. If the member is a row-major matrix with C columns and R rows, the
matrix is stored identically to an array of R row vectors with C
components each, according to rule (4)."
[...]
"When using the std430 storage layout, shader storage blocks will be
laid out in buffer storage identically to uniform and shader storage
blocks using the std140 layout, except that the base alignment and
stride of arrays of scalars and vectors in rule 4 and of structures in
rule 9 are not rounded up a multiple of the base alignment of a vec4."

In summary: vec2 has a base alignment of 2*N, a row-major mat2xY is
stored like an array of Y row vectors with 2 components each. Finally,
because of std430 storage layout, the base alignment of the array of
vectors is not rounded up to vec4, so it is still 2*N.

I can add this spec quote as a comment in the code, in the commit log or
both. What do you prefer?

I have not added the comment before because it is quite large, but I
think it makes sense to have it at least in the commit log.

Sam

>> +      if (packing == GLSL_INTERFACE_PACKING_STD430 && matrix_columns
>> == 2)
>> +         matrix_stride = 2 * N;
>> +      else
>> +         matrix_stride = glsl_align(matrix_columns * N, 16);
>>  
>>        const glsl_type *deref_type = deref->type->base_type ==
>> GLSL_TYPE_FLOAT ?
>>           glsl_type::float_type : glsl_type::double_type;
>