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

@@ -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

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
 ///

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;

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) {