Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix layout validation #5015

Merged
merged 2 commits into from
Dec 16, 2022
Merged

Fix layout validation #5015

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b87ca65
Fix layout validation
alan-baker Dec 13, 2022
8539745
Remove a dead if that was introduced
dneto0 Dec 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions source/val/validate_decorations.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,10 +633,10 @@ spv_result_t checkLayout(uint32_t struct_id, const char* storage_class_str,
}
}
nextValidOffset = offset + size;
if (!scalar_block_layout && blockRules &&
if (!scalar_block_layout &&
(spv::Op::OpTypeArray == opcode || spv::Op::OpTypeStruct == opcode)) {
// Uniform block rules don't permit anything in the padding of a struct
// or array.
// Non-scalar block layout rules don't permit anything in the padding of
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's what the spec says.

// a struct or array.
nextValidOffset = align(nextValidOffset, alignment);
}
}
Expand Down
49 changes: 46 additions & 3 deletions test/val/val_decoration_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4520,6 +4520,48 @@ TEST_F(ValidateDecorations, ArrayArrayRowMajorMatrixNextMemberOverlapsBad) {
TEST_F(ValidateDecorations, StorageBufferArraySizeCalculationPackGood) {
// Original GLSL

// #version 450
// layout (set=0,binding=0) buffer S {
// uvec3 arr[2][2]; // first 3 elements are 16 bytes, last is 12
// uint i; // Can't have offset 60 = 3x16 + 12
// } B;
// void main() {}

std::string spirv = R"(
OpCapability Shader
OpMemoryModel Logical GLSL450
OpEntryPoint Vertex %1 "main"
OpDecorate %_arr_v3uint_uint_2 ArrayStride 16
OpDecorate %_arr__arr_v3uint_uint_2_uint_2 ArrayStride 32
OpMemberDecorate %_struct_4 0 Offset 0
OpMemberDecorate %_struct_4 1 Offset 64
OpDecorate %_struct_4 BufferBlock
OpDecorate %5 DescriptorSet 0
OpDecorate %5 Binding 0
%void = OpTypeVoid
%7 = OpTypeFunction %void
%uint = OpTypeInt 32 0
%v3uint = OpTypeVector %uint 3
%uint_2 = OpConstant %uint 2
%_arr_v3uint_uint_2 = OpTypeArray %v3uint %uint_2
%_arr__arr_v3uint_uint_2_uint_2 = OpTypeArray %_arr_v3uint_uint_2 %uint_2
%_struct_4 = OpTypeStruct %_arr__arr_v3uint_uint_2_uint_2 %uint
%_ptr_Uniform__struct_4 = OpTypePointer Uniform %_struct_4
%5 = OpVariable %_ptr_Uniform__struct_4 Uniform
%1 = OpFunction %void None %7
%12 = OpLabel
OpReturn
OpFunctionEnd
)";

CompileSuccessfully(spirv);
EXPECT_EQ(SPV_SUCCESS,
ValidateAndRetrieveValidationState(SPV_ENV_VULKAN_1_0));
}

TEST_F(ValidateDecorations, StorageBufferArraySizeCalculationPackGoodScalar) {
// Original GLSL

// #version 450
// layout (set=0,binding=0) buffer S {
// uvec3 arr[2][2]; // first 3 elements are 16 bytes, last is 12
Expand Down Expand Up @@ -4554,6 +4596,7 @@ TEST_F(ValidateDecorations, StorageBufferArraySizeCalculationPackGood) {
OpFunctionEnd
)";

options_->scalar_block_layout = true;
CompileSuccessfully(spirv);
EXPECT_EQ(SPV_SUCCESS,
ValidateAndRetrieveValidationState(SPV_ENV_VULKAN_1_0));
Expand All @@ -4569,7 +4612,7 @@ TEST_F(ValidateDecorations, StorageBufferArraySizeCalculationPackBad) {
OpDecorate %_arr_v3uint_uint_2 ArrayStride 16
OpDecorate %_arr__arr_v3uint_uint_2_uint_2 ArrayStride 32
OpMemberDecorate %_struct_4 0 Offset 0
OpMemberDecorate %_struct_4 1 Offset 56
OpMemberDecorate %_struct_4 1 Offset 60
OpDecorate %_struct_4 BufferBlock
OpDecorate %5 DescriptorSet 0
OpDecorate %5 Binding 0
Expand All @@ -4595,8 +4638,8 @@ TEST_F(ValidateDecorations, StorageBufferArraySizeCalculationPackBad) {
EXPECT_THAT(getDiagnosticString(),
HasSubstr("Structure id 4 decorated as BufferBlock for variable "
"in Uniform storage class must follow standard storage "
"buffer layout rules: member 1 at offset 56 overlaps "
"previous member ending at offset 59"));
"buffer layout rules: member 1 at offset 60 overlaps "
"previous member ending at offset 63"));
}

TEST_F(ValidateDecorations, UniformBufferArraySizeCalculationPackGood) {
Expand Down