diff --git a/source/val/validate_decorations.cpp b/source/val/validate_decorations.cpp index c1fca45f9..605b79ebf 100644 --- a/source/val/validate_decorations.cpp +++ b/source/val/validate_decorations.cpp @@ -452,7 +452,16 @@ spv_result_t checkLayout(uint32_t struct_id, const char* storage_class_str, return ds; }; - const auto& members = getStructMembers(struct_id, vstate); + // If we are checking physical storage buffer pointers, we may not actually + // have a struct here. Instead, pretend we have a struct with a single member + // at offset 0. + const auto& struct_type = vstate.FindDef(struct_id); + std::vector members; + if (struct_type->opcode() == spv::Op::OpTypeStruct) { + members = getStructMembers(struct_id, vstate); + } else { + members.push_back(struct_id); + } // To check for member overlaps, we want to traverse the members in // offset order. @@ -461,31 +470,38 @@ spv_result_t checkLayout(uint32_t struct_id, const char* storage_class_str, uint32_t offset; }; std::vector member_offsets; - member_offsets.reserve(members.size()); - for (uint32_t memberIdx = 0, numMembers = uint32_t(members.size()); - memberIdx < numMembers; memberIdx++) { - uint32_t offset = 0xffffffff; - auto member_decorations = - vstate.id_member_decorations(struct_id, memberIdx); - for (auto decoration = member_decorations.begin; - decoration != member_decorations.end; ++decoration) { - assert(decoration->struct_member_index() == (int)memberIdx); - switch (decoration->dec_type()) { - case spv::Decoration::Offset: - offset = decoration->params()[0]; - break; - default: - break; + + // With physical storage buffers, we might be checking layouts that do not + // originate from a structure. + if (struct_type->opcode() == spv::Op::OpTypeStruct) { + member_offsets.reserve(members.size()); + for (uint32_t memberIdx = 0, numMembers = uint32_t(members.size()); + memberIdx < numMembers; memberIdx++) { + uint32_t offset = 0xffffffff; + auto member_decorations = + vstate.id_member_decorations(struct_id, memberIdx); + for (auto decoration = member_decorations.begin; + decoration != member_decorations.end; ++decoration) { + assert(decoration->struct_member_index() == (int)memberIdx); + switch (decoration->dec_type()) { + case spv::Decoration::Offset: + offset = decoration->params()[0]; + break; + default: + break; + } } + member_offsets.push_back( + MemberOffsetPair{memberIdx, incoming_offset + offset}); } - member_offsets.push_back( - MemberOffsetPair{memberIdx, incoming_offset + offset}); + std::stable_sort( + member_offsets.begin(), member_offsets.end(), + [](const MemberOffsetPair& lhs, const MemberOffsetPair& rhs) { + return lhs.offset < rhs.offset; + }); + } else { + member_offsets.push_back({0, 0}); } - std::stable_sort( - member_offsets.begin(), member_offsets.end(), - [](const MemberOffsetPair& lhs, const MemberOffsetPair& rhs) { - return lhs.offset < rhs.offset; - }); // Now scan from lowest offset to highest offset. uint32_t nextValidOffset = 0; @@ -1023,6 +1039,8 @@ spv_result_t CheckDecorationsOfBuffers(ValidationState_t& vstate) { std::unordered_set uses_push_constant; for (const auto& inst : vstate.ordered_instructions()) { const auto& words = inst.words(); + auto type_id = inst.type_id(); + const Instruction* type_inst = vstate.FindDef(type_id); if (spv::Op::OpVariable == inst.opcode()) { const auto var_id = inst.id(); // For storage class / decoration combinations, see Vulkan 14.5.4 "Offset @@ -1276,6 +1294,23 @@ spv_result_t CheckDecorationsOfBuffers(ValidationState_t& vstate) { } } } + } else if (type_inst && type_inst->opcode() == spv::Op::OpTypePointer && + type_inst->GetOperandAs(1u) == + spv::StorageClass::PhysicalStorageBuffer) { + const bool scalar_block_layout = vstate.options()->scalar_block_layout; + MemberConstraints constraints; + const bool buffer = true; + const auto data_type_id = type_inst->GetOperandAs(2u); + const auto* data_type_inst = vstate.FindDef(data_type_id); + if (data_type_inst->opcode() == spv::Op::OpTypeStruct) { + ComputeMemberConstraintsForStruct(&constraints, data_type_id, + LayoutConstraints(), vstate); + } + if (auto res = checkLayout(data_type_id, "PhysicalStorageBuffer", "Block", + !buffer, scalar_block_layout, 0, constraints, + vstate)) { + return res; + } } } return SPV_SUCCESS; diff --git a/test/opt/inst_buff_addr_check_test.cpp b/test/opt/inst_buff_addr_check_test.cpp index 9484b5a20..99f88f44c 100644 --- a/test/opt/inst_buff_addr_check_test.cpp +++ b/test/opt/inst_buff_addr_check_test.cpp @@ -928,6 +928,7 @@ OpFunctionEnd SetTargetEnv(SPV_ENV_VULKAN_1_2); SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS); + ValidatorOptions()->scalar_block_layout = true; SinglePassRunAndMatch(text, true, 7, 23); } diff --git a/test/val/val_decoration_test.cpp b/test/val/val_decoration_test.cpp index be16abae9..7febb6974 100644 --- a/test/val/val_decoration_test.cpp +++ b/test/val/val_decoration_test.cpp @@ -9252,6 +9252,124 @@ OpFunctionEnd EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_VULKAN_1_0)); } +TEST_F(ValidateDecorations, PhysicalStorageBufferWithOffset) { + const std::string spirv = R"( +OpCapability Shader +OpCapability Int64 +OpCapability PhysicalStorageBufferAddresses +OpMemoryModel PhysicalStorageBuffer64 GLSL450 +OpEntryPoint GLCompute %main "main" %pc +OpExecutionMode %main LocalSize 1 1 1 +OpDecorate %pc_block Block +OpMemberDecorate %pc_block 0 Offset 0 +OpMemberDecorate %pssbo_struct 0 Offset 0 +%void = OpTypeVoid +%long = OpTypeInt 64 0 +%float = OpTypeFloat 32 +%int = OpTypeInt 32 0 +%int_0 = OpConstant %int 0 +%pc_block = OpTypeStruct %long +%pc_block_ptr = OpTypePointer PushConstant %pc_block +%pc_long_ptr = OpTypePointer PushConstant %long +%pc = OpVariable %pc_block_ptr PushConstant +%pssbo_struct = OpTypeStruct %float +%pssbo_ptr = OpTypePointer PhysicalStorageBuffer %pssbo_struct +%void_fn = OpTypeFunction %void +%main = OpFunction %void None %void_fn +%entry = OpLabel +%pc_gep = OpAccessChain %pc_long_ptr %pc %int_0 +%addr = OpLoad %long %pc_gep +%ptr = OpConvertUToPtr %pssbo_ptr %addr +OpReturn +OpFunctionEnd +)"; + + CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_3); + EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_VULKAN_1_3)); +} + +TEST_F(ValidateDecorations, PhysicalStorageBufferMissingOffset) { + const std::string spirv = R"( +OpCapability Shader +OpCapability Int64 +OpCapability PhysicalStorageBufferAddresses +OpMemoryModel PhysicalStorageBuffer64 GLSL450 +OpEntryPoint GLCompute %main "main" %pc +OpExecutionMode %main LocalSize 1 1 1 +OpDecorate %pc_block Block +OpMemberDecorate %pc_block 0 Offset 0 +%void = OpTypeVoid +%long = OpTypeInt 64 0 +%float = OpTypeFloat 32 +%int = OpTypeInt 32 0 +%int_0 = OpConstant %int 0 +%pc_block = OpTypeStruct %long +%pc_block_ptr = OpTypePointer PushConstant %pc_block +%pc_long_ptr = OpTypePointer PushConstant %long +%pc = OpVariable %pc_block_ptr PushConstant +%pssbo_struct = OpTypeStruct %float +%pssbo_ptr = OpTypePointer PhysicalStorageBuffer %pssbo_struct +%void_fn = OpTypeFunction %void +%main = OpFunction %void None %void_fn +%entry = OpLabel +%pc_gep = OpAccessChain %pc_long_ptr %pc %int_0 +%addr = OpLoad %long %pc_gep +%ptr = OpConvertUToPtr %pssbo_ptr %addr +OpReturn +OpFunctionEnd +)"; + + CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_3); + EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_3)); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr("decorated as Block for variable in PhysicalStorageBuffer " + "storage class must follow relaxed storage buffer layout " + "rules: member 0 is missing an Offset decoration")); +} + +TEST_F(ValidateDecorations, PhysicalStorageBufferMissingArrayStride) { + const std::string spirv = R"( +OpCapability Shader +OpCapability Int64 +OpCapability PhysicalStorageBufferAddresses +OpMemoryModel PhysicalStorageBuffer64 GLSL450 +OpEntryPoint GLCompute %main "main" %pc +OpExecutionMode %main LocalSize 1 1 1 +OpDecorate %pc_block Block +OpMemberDecorate %pc_block 0 Offset 0 +%void = OpTypeVoid +%long = OpTypeInt 64 0 +%float = OpTypeFloat 32 +%int = OpTypeInt 32 0 +%int_0 = OpConstant %int 0 +%int_4 = OpConstant %int 4 +%pc_block = OpTypeStruct %long +%pc_block_ptr = OpTypePointer PushConstant %pc_block +%pc_long_ptr = OpTypePointer PushConstant %long +%pc = OpVariable %pc_block_ptr PushConstant +%pssbo_array = OpTypeArray %float %int_4 +%pssbo_ptr = OpTypePointer PhysicalStorageBuffer %pssbo_array +%void_fn = OpTypeFunction %void +%main = OpFunction %void None %void_fn +%entry = OpLabel +%pc_gep = OpAccessChain %pc_long_ptr %pc %int_0 +%addr = OpLoad %long %pc_gep +%ptr = OpConvertUToPtr %pssbo_ptr %addr +OpReturn +OpFunctionEnd +)"; + + CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_3); + EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_3)); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr( + "decorated as Block for variable in PhysicalStorageBuffer storage " + "class must follow relaxed storage buffer layout rules: member 0 " + "contains an array with stride 0, but with an element size of 4")); +} + } // namespace } // namespace val } // namespace spvtools