[Piglit] [PATCH v3 4/5] arb-enhanced-layouts: explicit-offset: test offset wrt (self) alignment

[Emil Velikov]

On 11 November 2015 at 21:10, Timothy Arceri <t_arceri at yahoo.com.au> wrote:
> On Wed, 2015-11-11 at 18:14 +0000, Emil Velikov wrote:
>> From: Emil Velikov <emil.velikov at collabora.com>
>>
>> As per the spec hunk:
>>    "The specified offset must be a
>>    multiple of the base alignment of the type of the block member it
>>    qualifies, or a compile-time error results."
>>
>> v2:
>>  - Fix typo - enhanced-layout > enhanced-layouts
>>  - Prefix uniform tests with ubo
>>  - Add ssbo equivalent tests
>>  - Quote the correct spec hunk in commit msg
>>
>> v3:
>>  - Remove trailing whitespace (Tim)
>>  - Drop glsl versoin to 1.40 for the ssbo tests (Tim, Ilia)
>>  - Change check_link to true to match below test results (Tim)
>
> Again I think you had this right the first time, the spec clearly says compile
> -time error. Sorry for the confusion.
>
Nothing to apologise for. I've completely missed your reply on the
topic :-\ Will revert to previous behaviour.

>>
>> Test results (Tim):
>> Nvidia GeForce 840M - NVIDIA 352.41: pass
>>
>> Signed-off-by: Emil Velikov <emil.l.velikov at gmail.com>
>> ---
>>  .../ssbo-offset-multiple-of-base-member-align.vert | 27
>> ++++++++++++++++++++++
>>  .../ubo-offset-multiple-of-base-member-align.vert  | 26
>> +++++++++++++++++++++
>>  2 files changed, 53 insertions(+)
>>  create mode 100644 tests/spec/arb_enhanced_layouts/compiler/explicit
>> -offsets/ssbo-offset-multiple-of-base-member-align.vert
>>  create mode 100644 tests/spec/arb_enhanced_layouts/compiler/explicit
>> -offsets/ubo-offset-multiple-of-base-member-align.vert
>>
>> diff --git a/tests/spec/arb_enhanced_layouts/compiler/explicit-offsets/ssbo
>> -offset-multiple-of-base-member-align.vert
>> b/tests/spec/arb_enhanced_layouts/compiler/explicit-offsets/ssbo-offset
>> -multiple-of-base-member-align.vert
>> new file mode 100644
>> index 0000000..231d445
>> --- /dev/null
>> +++ b/tests/spec/arb_enhanced_layouts/compiler/explicit-offsets/ssbo-offset
>> -multiple-of-base-member-align.vert
>> @@ -0,0 +1,27 @@
>> +// [config]
>> +// expect_result: fail
>> +// glsl_version: 1.40
>> +// require_extensions: GL_ARB_enhanced_layouts
>> GL_ARB_shader_storage_buffer_object
>> +// check_link: true
>> +// [end config]
>> +//
>> +// ARB_enhanced_layouts spec says:
>> +//    "The specified offset must be a
>> +//    multiple of the base alignment of the type of the block member it
>> +//    qualifies, or a compile-time error results."
>> +//
>> +// Tests for successful compilation, when the block is of std140 layout.
>> +//
>> +
>> +#version 140
>> +#extension GL_ARB_enhanced_layouts : enable
>> +#extension GL_ARB_shader_storage_buffer_object : enable
>> +
>> +layout(std430) buffer b {
>> +       layout(offset = 0) float f1;
>> +       layout(offset = 2) float f2; // Wrong: offset must be aligned to
>
>
> I think this should be:
>
> layout(offset = 6) float f2;
>
> Otherwise the test could pass because the offset lies within the previous
> member of the block not because the alignment is wrong.
>
I believe you mean "Otherwise the test _should fail_ ...." in the
above. Although you're spot on - we should be checking the offset
alignment here, not if members overlap.

> Same goes for UBOs.
>
> With these changes and removing the link time check.
>
> Reviewed-by: Timothy Arceri <timothy.arceri at collabora.com>
>
Thanks
Emil