Skip to content

Commit

Permalink
Require ColMajor or RowMajor for matrices (#4878)
Browse files Browse the repository at this point in the history 
Fixes #4875

* Require that matrices in laid out structs have RowMajor or ColMajor
  set as per SPIR-V section 2.16.2 (shader validation)
  • Loading branch information
@alan-baker
alan-baker authored Jul 29, 2022
1 parent a90ccc2 commit b5d0bf2
Show file tree
Hide file tree
Showing 2 changed files with 169 additions and 17 deletions.
53 changes: 36 additions & 17 deletions source/val/validate_decorations.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -660,18 +660,19 @@ bool hasDecoration(uint32_t id, SpvDecoration decoration,
}

// Returns true if all ids of given type have a specified decoration.
bool checkForRequiredDecoration(uint32_t struct_id, SpvDecoration decoration,
bool checkForRequiredDecoration(uint32_t struct_id,
std::function<bool(SpvDecoration)> checker,
SpvOp type, ValidationState_t& vstate) {
const auto& members = getStructMembers(struct_id, vstate);
for (size_t memberIdx = 0; memberIdx < members.size(); memberIdx++) {
const auto id = members[memberIdx];
if (type != vstate.FindDef(id)->opcode()) continue;
bool found = false;
for (auto& dec : vstate.id_decorations(id)) {
if (decoration == dec.dec_type()) found = true;
if (checker(dec.dec_type())) found = true;
}
for (auto& dec : vstate.id_decorations(struct_id)) {
if (decoration == dec.dec_type() &&
if (checker(dec.dec_type()) &&
(int)memberIdx == dec.struct_member_index()) {
found = true;
}
Expand All @@ -681,7 +682,7 @@ bool checkForRequiredDecoration(uint32_t struct_id, SpvDecoration decoration,
}
}
for (auto id : getStructMembers(struct_id, SpvOpTypeStruct, vstate)) {
if (!checkForRequiredDecoration(id, decoration, type, vstate)) {
if (!checkForRequiredDecoration(id, checker, type, vstate)) {
return false;
}
}
Expand Down Expand Up @@ -1223,30 +1224,48 @@ spv_result_t CheckDecorationsOfBuffers(ValidationState_t& vstate) {
return vstate.diag(SPV_ERROR_INVALID_ID, vstate.FindDef(id))
<< "Structure id " << id << " decorated as " << deco_str
<< " must not use GLSLPacked decoration.";
} else if (!checkForRequiredDecoration(id, SpvDecorationArrayStride,
SpvOpTypeArray, vstate)) {
} else if (!checkForRequiredDecoration(
id,
[](SpvDecoration d) {
return d == SpvDecorationArrayStride;
},
SpvOpTypeArray, vstate)) {
return vstate.diag(SPV_ERROR_INVALID_ID, vstate.FindDef(id))
<< "Structure id " << id << " decorated as " << deco_str
<< " must be explicitly laid out with ArrayStride "
"decorations.";
} else if (!checkForRequiredDecoration(id,
SpvDecorationMatrixStride,
SpvOpTypeMatrix, vstate)) {
} else if (!checkForRequiredDecoration(
id,
[](SpvDecoration d) {
return d == SpvDecorationMatrixStride;
},
SpvOpTypeMatrix, vstate)) {
return vstate.diag(SPV_ERROR_INVALID_ID, vstate.FindDef(id))
<< "Structure id " << id << " decorated as " << deco_str
<< " must be explicitly laid out with MatrixStride "
"decorations.";
} else if (!checkForRequiredDecoration(
id,
[](SpvDecoration d) {
return d == SpvDecorationRowMajor ||
d == SpvDecorationColMajor;
},
SpvOpTypeMatrix, vstate)) {
return vstate.diag(SPV_ERROR_INVALID_ID, vstate.FindDef(id))
<< "Structure id " << id << " decorated as " << deco_str
<< " must be explicitly laid out with RowMajor or "
"ColMajor decorations.";
} else if (blockRules &&
(SPV_SUCCESS != (recursive_status = checkLayout(
id, sc_str, deco_str, true,
scalar_block_layout, 0,
constraints, vstate)))) {
(SPV_SUCCESS !=
(recursive_status = checkLayout(
id, sc_str, deco_str, true, scalar_block_layout, 0,
constraints, vstate)))) {
return recursive_status;
} else if (bufferRules &&
(SPV_SUCCESS != (recursive_status = checkLayout(
id, sc_str, deco_str, false,
scalar_block_layout, 0,
constraints, vstate)))) {
(SPV_SUCCESS !=
(recursive_status = checkLayout(
id, sc_str, deco_str, false, scalar_block_layout,
0, constraints, vstate)))) {
return recursive_status;
}
}
Expand Down
133 changes: 133 additions & 0 deletions test/val/val_decoration_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8023,6 +8023,7 @@ OpExecutionMode %main LocalSize 1 1 1
OpDecorate %block Block
OpMemberDecorate %block 0 Offset 0
OpMemberDecorate %block 0 MatrixStride 3
OpMemberDecorate %block 0 ColMajor
OpDecorate %var DescriptorSet 0
OpDecorate %var Binding 0
%void = OpTypeVoid
Expand Down Expand Up @@ -8059,6 +8060,7 @@ OpExecutionMode %main LocalSize 1 1 1
OpDecorate %block Block
OpMemberDecorate %block 0 Offset 0
OpMemberDecorate %block 0 MatrixStride 3
OpMemberDecorate %block 0 ColMajor
OpDecorate %var DescriptorSet 0
OpDecorate %var Binding 0
%void = OpTypeVoid
Expand Down Expand Up @@ -8094,6 +8096,7 @@ OpExecutionMode %main LocalSize 1 1 1
OpDecorate %block Block
OpMemberDecorate %block 0 Offset 0
OpMemberDecorate %block 0 MatrixStride 3
OpMemberDecorate %block 0 ColMajor
OpDecorate %var DescriptorSet 0
OpDecorate %var Binding 0
%void = OpTypeVoid
Expand Down Expand Up @@ -8130,6 +8133,7 @@ OpExecutionMode %main LocalSize 1 1 1
OpDecorate %block Block
OpMemberDecorate %block 0 Offset 0
OpMemberDecorate %block 0 MatrixStride 3
OpMemberDecorate %block 0 RowMajor
OpDecorate %var DescriptorSet 0
OpDecorate %var Binding 0
%void = OpTypeVoid
Expand Down Expand Up @@ -8871,6 +8875,135 @@ OpFunctionEnd
"alignment to 16"));
}

TEST_F(ValidateDecorations, MatrixMissingMajornessUniform) {
const std::string spirv = R"(
OpCapability Shader
OpMemoryModel Logical GLSL450
OpEntryPoint GLCompute %main "main"
OpExecutionMode %main LocalSize 1 1 1
OpDecorate %block Block
OpMemberDecorate %block 0 Offset 0
OpMemberDecorate %block 0 MatrixStride 16
OpDecorate %var DescriptorSet 0
OpDecorate %var Binding 0
%void = OpTypeVoid
%void_fn = OpTypeFunction %void
%float = OpTypeFloat 32
%float2 = OpTypeVector %float 2
%matrix = OpTypeMatrix %float2 2
%block = OpTypeStruct %matrix
%ptr_block = OpTypePointer Uniform %block
%var = OpVariable %ptr_block Uniform
%main = OpFunction %void None %void_fn
%entry = OpLabel
OpReturn
OpFunctionEnd
)";

CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_0);
EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_0));
EXPECT_THAT(
getDiagnosticString(),
HasSubstr(
"must be explicitly laid out with RowMajor or ColMajor decorations"));
}

TEST_F(ValidateDecorations, MatrixMissingMajornessStorageBuffer) {
const std::string spirv = R"(
OpCapability Shader
OpExtension "SPV_KHR_storage_buffer_storage_class"
OpMemoryModel Logical GLSL450
OpEntryPoint GLCompute %main "main"
OpExecutionMode %main LocalSize 1 1 1
OpDecorate %block Block
OpMemberDecorate %block 0 Offset 0
OpMemberDecorate %block 0 MatrixStride 16
OpDecorate %var DescriptorSet 0
OpDecorate %var Binding 0
%void = OpTypeVoid
%void_fn = OpTypeFunction %void
%float = OpTypeFloat 32
%float2 = OpTypeVector %float 2
%matrix = OpTypeMatrix %float2 2
%block = OpTypeStruct %matrix
%ptr_block = OpTypePointer StorageBuffer %block
%var = OpVariable %ptr_block StorageBuffer
%main = OpFunction %void None %void_fn
%entry = OpLabel
OpReturn
OpFunctionEnd
)";

CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_0);
EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_0));
EXPECT_THAT(
getDiagnosticString(),
HasSubstr(
"must be explicitly laid out with RowMajor or ColMajor decorations"));
}

TEST_F(ValidateDecorations, MatrixMissingMajornessPushConstant) {
const std::string spirv = R"(
OpCapability Shader
OpMemoryModel Logical GLSL450
OpEntryPoint GLCompute %main "main"
OpExecutionMode %main LocalSize 1 1 1
OpDecorate %block Block
OpMemberDecorate %block 0 Offset 0
OpMemberDecorate %block 0 MatrixStride 16
%void = OpTypeVoid
%void_fn = OpTypeFunction %void
%float = OpTypeFloat 32
%float2 = OpTypeVector %float 2
%matrix = OpTypeMatrix %float2 2
%block = OpTypeStruct %matrix
%ptr_block = OpTypePointer PushConstant %block
%var = OpVariable %ptr_block PushConstant
%main = OpFunction %void None %void_fn
%entry = OpLabel
OpReturn
OpFunctionEnd
)";

CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_0);
EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_0));
EXPECT_THAT(
getDiagnosticString(),
HasSubstr(
"must be explicitly laid out with RowMajor or ColMajor decorations"));
}

TEST_F(ValidateDecorations, StructWithRowAndColMajor) {
const std::string spirv = R"(
OpCapability Shader
OpMemoryModel Logical GLSL450
OpEntryPoint GLCompute %main "main"
OpExecutionMode %main LocalSize 1 1 1
OpDecorate %block Block
OpMemberDecorate %block 0 Offset 0
OpMemberDecorate %block 0 MatrixStride 16
OpMemberDecorate %block 0 ColMajor
OpMemberDecorate %block 1 Offset 32
OpMemberDecorate %block 1 MatrixStride 16
OpMemberDecorate %block 1 RowMajor
%void = OpTypeVoid
%void_fn = OpTypeFunction %void
%float = OpTypeFloat 32
%float2 = OpTypeVector %float 2
%matrix = OpTypeMatrix %float2 2
%block = OpTypeStruct %matrix %matrix
%ptr_block = OpTypePointer PushConstant %block
%var = OpVariable %ptr_block PushConstant
%main = OpFunction %void None %void_fn
%entry = OpLabel
OpReturn
OpFunctionEnd
)";

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

} // namespace
} // namespace val
} // namespace spvtools

0 comments on commit b5d0bf2

Please sign in to comment.