From f0aa6f4e3a57d428725ef91b7005bb4371d78155 Mon Sep 17 00:00:00 2001 From: Jaebaek Seo Date: Thu, 27 Sep 2018 11:09:47 -0400 Subject: [PATCH] Fixed Validator adjacency bug for OpPhi (#1922) OpPhi instruction must appear before all non-OpPhi instructions except for OpLine. Without this commit, Validator does not check the case that an OpPhi is preceeded by an OpLine and the OpLine is preceeded by a non-OpPhi instruction that is not OpLine. --- source/val/validate.cpp | 7 +- source/val/validate.h | 2 +- source/val/validate_adjacency.cpp | 109 ++++++++++++++++------------ test/val/val_adjacency_test.cpp | 113 +++++++++++++++++++++++++++++- 4 files changed, 180 insertions(+), 51 deletions(-) diff --git a/source/val/validate.cpp b/source/val/validate.cpp index 012e9d70..cd346651 100644 --- a/source/val/validate.cpp +++ b/source/val/validate.cpp @@ -332,11 +332,12 @@ spv_result_t ValidateBinaryUsingContextAndValidationState( if (auto error = NonUniformPass(*vstate, &instruction)) return error; if (auto error = LiteralsPass(*vstate, &instruction)) return error; - // Validate the preconditions involving adjacent instructions. e.g. SpvOpPhi - // must only be preceeded by SpvOpLabel, SpvOpPhi, or SpvOpLine. - if (auto error = ValidateAdjacency(*vstate, i)) return error; } + // Validate the preconditions involving adjacent instructions. e.g. SpvOpPhi + // must only be preceeded by SpvOpLabel, SpvOpPhi, or SpvOpLine. + if (auto error = ValidateAdjacency(*vstate)) return error; + if (auto error = ValidateEntryPoints(*vstate)) return error; // CFG checks are performed after the binary has been parsed // and the CFGPass has collected information about the control flow diff --git a/source/val/validate.h b/source/val/validate.h index 518547f0..276746de 100644 --- a/source/val/validate.h +++ b/source/val/validate.h @@ -75,7 +75,7 @@ spv_result_t CheckIdDefinitionDominateUse(const ValidationState_t& _); /// @param[in] _ the validation state of the module /// /// @return SPV_SUCCESS if no errors are found. SPV_ERROR_INVALID_DATA otherwise -spv_result_t ValidateAdjacency(ValidationState_t& _, size_t idx); +spv_result_t ValidateAdjacency(ValidationState_t& _); /// @brief Validates static uses of input and output variables /// diff --git a/source/val/validate_adjacency.cpp b/source/val/validate_adjacency.cpp index 5ef56be9..ae2eff3f 100644 --- a/source/val/validate_adjacency.cpp +++ b/source/val/validate_adjacency.cpp @@ -27,56 +27,73 @@ namespace spvtools { namespace val { -spv_result_t ValidateAdjacency(ValidationState_t& _, size_t idx) { - const auto& instructions = _.ordered_instructions(); - const auto& inst = instructions[idx]; +enum { + IN_NEW_FUNCTION, + IN_ENTRY_BLOCK, + PHI_VALID, + PHI_INVALID, +}; - switch (inst.opcode()) { - case SpvOpPhi: - if (idx > 0) { - switch (instructions[idx - 1].opcode()) { - case SpvOpLabel: - case SpvOpPhi: - case SpvOpLine: - break; - default: - return _.diag(SPV_ERROR_INVALID_DATA, &inst) - << "OpPhi must appear before all non-OpPhi instructions " - << "(except for OpLine, which can be mixed with OpPhi)."; +spv_result_t ValidateAdjacency(ValidationState_t& _) { + const auto& instructions = _.ordered_instructions(); + int phi_check = PHI_INVALID; + + for (size_t i = 0; i < instructions.size(); ++i) { + const auto& inst = instructions[i]; + switch (inst.opcode()) { + case SpvOpFunction: + case SpvOpFunctionParameter: + phi_check = IN_NEW_FUNCTION; + break; + case SpvOpLabel: + phi_check = phi_check == IN_NEW_FUNCTION ? IN_ENTRY_BLOCK : PHI_VALID; + break; + case SpvOpPhi: + if (phi_check != PHI_VALID) { + return _.diag(SPV_ERROR_INVALID_DATA, &inst) + << "OpPhi must appear within a non-entry block before all " + << "non-OpPhi instructions " + << "(except for OpLine, which can be mixed with OpPhi)."; } - } - break; - case SpvOpLoopMerge: - if (idx != (instructions.size() - 1)) { - switch (instructions[idx + 1].opcode()) { - case SpvOpBranch: - case SpvOpBranchConditional: - break; - default: - return _.diag(SPV_ERROR_INVALID_DATA, &inst) - << "OpLoopMerge must immediately precede either an " - << "OpBranch or OpBranchConditional instruction. " - << "OpLoopMerge must be the second-to-last instruction in " - << "its block."; + break; + case SpvOpLine: + break; + case SpvOpLoopMerge: + phi_check = PHI_INVALID; + if (i != (instructions.size() - 1)) { + switch (instructions[i + 1].opcode()) { + case SpvOpBranch: + case SpvOpBranchConditional: + break; + default: + return _.diag(SPV_ERROR_INVALID_DATA, &inst) + << "OpLoopMerge must immediately precede either an " + << "OpBranch or OpBranchConditional instruction. " + << "OpLoopMerge must be the second-to-last instruction in " + << "its block."; + } } - } - break; - case SpvOpSelectionMerge: - if (idx != (instructions.size() - 1)) { - switch (instructions[idx + 1].opcode()) { - case SpvOpBranchConditional: - case SpvOpSwitch: - break; - default: - return _.diag(SPV_ERROR_INVALID_DATA, &inst) - << "OpSelectionMerge must immediately precede either an " - << "OpBranchConditional or OpSwitch instruction. " - << "OpSelectionMerge must be the second-to-last " - << "instruction in its block."; + break; + case SpvOpSelectionMerge: + phi_check = PHI_INVALID; + if (i != (instructions.size() - 1)) { + switch (instructions[i + 1].opcode()) { + case SpvOpBranchConditional: + case SpvOpSwitch: + break; + default: + return _.diag(SPV_ERROR_INVALID_DATA, &inst) + << "OpSelectionMerge must immediately precede either an " + << "OpBranchConditional or OpSwitch instruction. " + << "OpSelectionMerge must be the second-to-last " + << "instruction in its block."; + } } - } - default: - break; + break; + default: + phi_check = PHI_INVALID; + break; + } } return SPV_SUCCESS; diff --git a/test/val/val_adjacency_test.cpp b/test/val/val_adjacency_test.cpp index d6283051..e407f59c 100644 --- a/test/val/val_adjacency_test.cpp +++ b/test/val/val_adjacency_test.cpp @@ -112,6 +112,7 @@ std::string GenerateShaderCode( %true = OpConstantTrue %bool %false = OpConstantFalse %bool %zero = OpConstant %int 0 +%int_1 = OpConstant %int 1 %func = OpTypeFunction %void %main = OpFunction %void None %func %main_entry = OpLabel @@ -193,7 +194,117 @@ OpNop CompileSuccessfully(GenerateShaderCode(body)); EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions()); EXPECT_THAT(getDiagnosticString(), - HasSubstr("OpPhi must appear before all non-OpPhi instructions")); + HasSubstr("OpPhi must appear within a non-entry block before all " + "non-OpPhi instructions")); +} + +TEST_F(ValidateAdjacency, OpPhiPreceededByOpLineAndBadOpFail) { + const std::string body = R"( +OpSelectionMerge %end_label None +OpBranchConditional %true %true_label %false_label +%true_label = OpLabel +OpBranch %end_label +%false_label = OpLabel +OpBranch %end_label +%end_label = OpLabel +OpNop +OpLine %string 1 1 +%result = OpPhi %bool %true %true_label %false %false_label +)"; + + CompileSuccessfully(GenerateShaderCode(body)); + EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions()); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("OpPhi must appear within a non-entry block before all " + "non-OpPhi instructions")); +} + +TEST_F(ValidateAdjacency, OpPhiFollowedByOpLineGood) { + const std::string body = R"( +OpSelectionMerge %end_label None +OpBranchConditional %true %true_label %false_label +%true_label = OpLabel +OpBranch %end_label +%false_label = OpLabel +OpBranch %end_label +%end_label = OpLabel +%result = OpPhi %bool %true %true_label %false %false_label +OpLine %string 1 1 +OpNop +OpNop +OpLine %string 2 1 +OpNop +OpLine %string 3 1 +)"; + + CompileSuccessfully(GenerateShaderCode(body)); + EXPECT_EQ(SPV_SUCCESS, ValidateInstructions()); +} + +TEST_F(ValidateAdjacency, OpPhiMultipleOpLineAndOpPhiFail) { + const std::string body = R"( +OpSelectionMerge %end_label None +OpBranchConditional %true %true_label %false_label +%true_label = OpLabel +OpBranch %end_label +%false_label = OpLabel +OpBranch %end_label +%end_label = OpLabel +OpLine %string 1 1 +%value = OpPhi %int %zero %true_label %int_1 %false_label +OpNop +OpLine %string 2 1 +OpNop +OpLine %string 3 1 +%result = OpPhi %bool %true %true_label %false %false_label +)"; + + CompileSuccessfully(GenerateShaderCode(body)); + EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions()); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("OpPhi must appear within a non-entry block before all " + "non-OpPhi instructions")); +} + +TEST_F(ValidateAdjacency, OpPhiMultipleOpLineAndOpPhiGood) { + const std::string body = R"( +OpSelectionMerge %end_label None +OpBranchConditional %true %true_label %false_label +%true_label = OpLabel +OpBranch %end_label +%false_label = OpLabel +OpBranch %end_label +%end_label = OpLabel +OpLine %string 1 1 +%value = OpPhi %int %zero %true_label %int_1 %false_label +OpLine %string 2 1 +%result = OpPhi %bool %true %true_label %false %false_label +OpLine %string 3 1 +OpNop +OpNop +OpLine %string 4 1 +OpNop +)"; + + CompileSuccessfully(GenerateShaderCode(body)); + EXPECT_EQ(SPV_SUCCESS, ValidateInstructions()); +} + +TEST_F(ValidateAdjacency, OpPhiInEntryBlockBad) { + const std::string body = R"( +OpLine %string 1 1 +%value = OpPhi %int +OpLine %string 2 1 +OpNop +OpLine %string 3 1 +OpNop +)"; + + CompileSuccessfully(GenerateShaderCode(body)); + EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions()); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("OpPhi must appear within a non-entry block before all " + "non-OpPhi instructions")); } TEST_F(ValidateAdjacency, OpLoopMergePreceedsOpBranchSuccess) {