diff --git a/source/val/validate_annotation.cpp b/source/val/validate_annotation.cpp index eaca8f0f79..2545f2fa59 100644 --- a/source/val/validate_annotation.cpp +++ b/source/val/validate_annotation.cpp @@ -333,6 +333,14 @@ spv_result_t ValidateDecorate(ValidationState_t& _, const Instruction* inst) { } spv_result_t ValidateDecorateId(ValidationState_t& _, const Instruction* inst) { + const auto target_id = inst->GetOperandAs(0); + const auto target = _.FindDef(target_id); + if (target && spv::Op::OpDecorationGroup == target->opcode()) { + return _.diag(SPV_ERROR_INVALID_ID, inst) + << "OpMemberDecorate Target " << _.getIdName(target_id) + << " must not be an OpDecorationGroup instruction."; + } + const auto decoration = inst->GetOperandAs(1); if (!DecorationTakesIdParameters(decoration)) { return _.diag(SPV_ERROR_INVALID_ID, inst) @@ -340,6 +348,20 @@ spv_result_t ValidateDecorateId(ValidationState_t& _, const Instruction* inst) { "OpDecorateId"; } + for (uint32_t i = 2; i < inst->operands().size(); ++i) { + const auto param_id = inst->GetOperandAs(i); + const auto param = _.FindDef(param_id); + + // Both target and param are elements of ordered_instructions we can + // determine their relative positions in the SPIR-V module by comparing + // pointers. + if (target <= param) { + return _.diag(SPV_ERROR_INVALID_ID, inst) + << "Parameter " << _.getIdName(param_id) + << " must appear earlier in the binary than the target"; + } + } + // No member decorations take id parameters, so we don't bother checking if // we are using a member only decoration here. @@ -388,8 +410,7 @@ spv_result_t ValidateDecorationGroup(ValidationState_t& _, if (use->opcode() != spv::Op::OpDecorate && use->opcode() != spv::Op::OpGroupDecorate && use->opcode() != spv::Op::OpGroupMemberDecorate && - use->opcode() != spv::Op::OpName && - use->opcode() != spv::Op::OpDecorateId && !use->IsNonSemantic()) { + use->opcode() != spv::Op::OpName && !use->IsNonSemantic()) { return _.diag(SPV_ERROR_INVALID_ID, inst) << "Result id of OpDecorationGroup can only " << "be targeted by OpName, OpGroupDecorate, " diff --git a/test/val/val_decoration_test.cpp b/test/val/val_decoration_test.cpp index 7a19df616a..fa4e27cbce 100644 --- a/test/val/val_decoration_test.cpp +++ b/test/val/val_decoration_test.cpp @@ -5445,6 +5445,97 @@ OpFunctionEnd EXPECT_EQ(SPV_SUCCESS, ValidateInstructions()); } +// OpDecorateId + +TEST_F(ValidateDecorations, DecorateIdGood) { + const std::string spirv = R"( +OpCapability Shader +OpMemoryModel Logical Simple +OpEntryPoint GLCompute %main "main" +OpExecutionMode %main LocalSize 1 1 1 +OpName %subgroupscope "subgroupscope" +OpName %int0 "int0" +OpName %fn "fn" +OpDecorateId %int0 UniformId %subgroupscope +%void = OpTypeVoid +%float = OpTypeFloat 32 +%int = OpTypeInt 32 1 +%subgroupscope = OpConstant %int 3 +%int0 = OpConstantNull %int +%fn = OpTypeFunction %void +%main = OpFunction %void None %fn +%entry = OpLabel +OpReturn +OpFunctionEnd +)"; + + CompileSuccessfully(spirv, SPV_ENV_UNIVERSAL_1_4); + EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_UNIVERSAL_1_4)); + EXPECT_THAT(getDiagnosticString(), Eq("")); +} + +TEST_F(ValidateDecorations, DecorateIdGroupBad) { + const std::string spirv = R"( +OpCapability Shader +OpMemoryModel Logical Simple +OpEntryPoint GLCompute %main "main" +OpExecutionMode %main LocalSize 1 1 1 +OpName %subgroupscope "subgroupscope" +OpName %int0 "int0" +OpName %fn "fn" +OpName %group "group" +OpDecorateId %group UniformId %subgroupscope +%group = OpDecorationGroup +OpGroupDecorate %group %int0 +%void = OpTypeVoid +%float = OpTypeFloat 32 +%int = OpTypeInt 32 1 +%subgroupscope = OpConstant %int 3 +%int0 = OpConstantNull %int +%fn = OpTypeFunction %void +%main = OpFunction %void None %fn +%entry = OpLabel +OpReturn +OpFunctionEnd +)"; + + CompileSuccessfully(spirv, SPV_ENV_UNIVERSAL_1_4); + EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_UNIVERSAL_1_4)); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("must not be an OpDecorationGroup instruction.\n" + " OpDecorateId %group UniformId %subgroupscope")); +} + +TEST_F(ValidateDecorations, DecorateIdOutOfOrderBad) { + const std::string spirv = R"( +OpCapability Shader +OpMemoryModel Logical Simple +OpEntryPoint GLCompute %main "main" +OpExecutionMode %main LocalSize 1 1 1 +OpName %subgroupscope "subgroupscope" +OpName %int0 "int0" +OpName %fn "fn" +OpDecorateId %int0 UniformId %subgroupscope +%void = OpTypeVoid +%float = OpTypeFloat 32 +%int = OpTypeInt 32 1 +%int0 = OpConstantNull %int +%subgroupscope = OpConstant %int 3 +%fn = OpTypeFunction %void +%main = OpFunction %void None %fn +%entry = OpLabel +OpReturn +OpFunctionEnd +)"; + + CompileSuccessfully(spirv, SPV_ENV_UNIVERSAL_1_4); + EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_UNIVERSAL_1_4)); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("[%subgroupscope]' must appear earlier in the" + " binary than the target\n" + " OpDecorateId %int0 UniformId %subgroupscope")); +} + // Uniform and UniformId decorations TEST_F(ValidateDecorations, UniformDecorationGood) { @@ -5486,6 +5577,7 @@ OpName %subgroupscope "subgroupscope" OpName %call "call" OpName %myfunc "myfunc" OpName %int0 "int0" +OpName %int1 "int1" OpName %float0 "float0" OpName %fn "fn" )") + inst + @@ -5493,10 +5585,11 @@ OpName %fn "fn" %void = OpTypeVoid %float = OpTypeFloat 32 %int = OpTypeInt 32 1 -%int0 = OpConstantNull %int +%int1 = OpConstant %int 1 %int_99 = OpConstant %int 99 %subgroupscope = OpConstant %int 3 %float0 = OpConstantNull %float +%int0 = OpConstantNull %int %fn = OpTypeFunction %void %myfunc = OpFunction %void None %fn %myfuncentry = OpLabel @@ -5613,7 +5706,7 @@ TEST_F(ValidateDecorations, TEST_F(ValidateDecorations, UniformDecorationWithScopeIdV14VulkanEnv) { const std::string spirv = - ShaderWithUniformLikeDecoration("OpDecorateId %int0 UniformId %int0"); + ShaderWithUniformLikeDecoration("OpDecorateId %int0 UniformId %int1"); CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_1_SPIRV_1_4); EXPECT_EQ(SPV_ERROR_INVALID_DATA, @@ -10519,8 +10612,8 @@ const std::string kNodeShaderPostlude = R"( %node1 = OpConstantStringAMDX "node1" %node2 = OpConstantStringAMDX "node2" %S = OpTypeStruct -%_payloadarr_S = OpTypeNodePayloadArrayAMDX %S %_payloadarr_S_0 = OpTypeNodePayloadArrayAMDX %S +%_payloadarr_S = OpTypeNodePayloadArrayAMDX %S %bool = OpTypeBool %true = OpConstantTrue %bool %void = OpTypeVoid diff --git a/test/val/val_modes_test.cpp b/test/val/val_modes_test.cpp index 293940c80c..75a956c445 100644 --- a/test/val/val_modes_test.cpp +++ b/test/val/val_modes_test.cpp @@ -2543,8 +2543,8 @@ const std::string kNodeShaderPostlude = R"( %node1 = OpConstantStringAMDX "node1" %node2 = OpConstantStringAMDX "node2" %S = OpTypeStruct -%_payloadarr_S = OpTypeNodePayloadArrayAMDX %S %_payloadarr_S_0 = OpTypeNodePayloadArrayAMDX %S +%_payloadarr_S = OpTypeNodePayloadArrayAMDX %S %bool = OpTypeBool %true = OpConstantTrue %bool %void = OpTypeVoid