Combine ordered_instruction loops in validation. (#1782)

There are several validation passes which loop over all ordered
instructions. This CL combines those into a single loop, calling each
pass as needed.

5 files changed, 100 insertions, 96 deletions

diff --git a/source/val/validate.cpp b/source/val/validate.cpp
index 6084e483..a56019cb 100644
--- a/source/val/validate.cpp
+++ b/source/val/validate.cpp
@@ -240,6 +240,18 @@ spv_result_t ValidateBinaryUsingContextAndValidationState(
     return error;
   }
 
+  for (size_t i = 0; i < vstate->ordered_instructions().size(); ++i) {
+    const auto& instruction = vstate->ordered_instructions()[i];
+
+    if (auto error = UpdateIdUse(*vstate, &instruction)) return error;
+    if (auto error = ValidateMemoryInstructions(*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;
+  }
+
   if (!vstate->has_memory_model_specified())
     return vstate->diag(SPV_ERROR_INVALID_LAYOUT, nullptr)
            << "Missing required OpMemoryModel instruction.";
@@ -268,18 +280,13 @@ spv_result_t ValidateBinaryUsingContextAndValidationState(
 
   vstate->ComputeFunctionToEntryPointMapping();
 
-  // 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;
-
   // CFG checks are performed after the binary has been parsed
   // and the CFGPass has collected information about the control flow
   if (auto error = PerformCfgChecks(*vstate)) return error;
-  if (auto error = UpdateIdUse(*vstate)) return error;
   if (auto error = CheckIdDefinitionDominateUse(*vstate)) return error;
   if (auto error = ValidateDecorations(*vstate)) return error;
   if (auto error = ValidateInterfaces(*vstate)) return error;
-  if (auto error = ValidateMemoryInstructions(*vstate)) return error;
+  if (auto error = ValidateBuiltIns(*vstate)) return error;
 
   // Entry point validation. Based on 2.16.1 (Universal Validation Rules) of the
   // SPIRV spec:
@@ -325,8 +332,6 @@ spv_result_t ValidateBinaryUsingContextAndValidationState(
                                   *vstate, &position))
     return error;
 
-  if (auto error = ValidateBuiltIns(*vstate)) return error;
-
   return SPV_SUCCESS;
 }
 
diff --git a/source/val/validate.h b/source/val/validate.h
index 33742e5c..654e87f3 100644
--- a/source/val/validate.h
+++ b/source/val/validate.h
@@ -51,7 +51,7 @@ spv_result_t PerformCfgChecks(ValidationState_t& _);
 /// @param[in] _ the validation state of the module
 ///
 /// @return SPV_SUCCESS if no errors are found.
-spv_result_t UpdateIdUse(ValidationState_t& _);
+spv_result_t UpdateIdUse(ValidationState_t& _, const Instruction* inst);
 
 /// @brief This function checks all ID definitions dominate their use in the
 /// CFG.
@@ -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& _);
+spv_result_t ValidateAdjacency(ValidationState_t& _, size_t idx);
 
 /// @brief Validates static uses of input and output variables
 ///
@@ -91,7 +91,8 @@ spv_result_t ValidateInterfaces(ValidationState_t& _);
 ///
 /// @param[in] _ the validation state of the module
 /// @return SPV_SUCCESS if no errors are found.
-spv_result_t ValidateMemoryInstructions(ValidationState_t& _);
+spv_result_t ValidateMemoryInstructions(ValidationState_t& _,
+                                        const Instruction* inst);
 
 /// @brief Updates the immediate dominator for each of the block edges
 ///
diff --git a/source/val/validate_adjacency.cpp b/source/val/validate_adjacency.cpp
index e19e30e5..5ef56be9 100644
--- a/source/val/validate_adjacency.cpp
+++ b/source/val/validate_adjacency.cpp
@@ -27,56 +27,56 @@
 namespace spvtools {
 namespace val {
 
-spv_result_t ValidateAdjacency(ValidationState_t& _) {
+spv_result_t ValidateAdjacency(ValidationState_t& _, size_t idx) {
   const auto& instructions = _.ordered_instructions();
-  for (auto i = instructions.cbegin(); i != instructions.cend(); ++i) {
-    switch (i->opcode()) {
-      case SpvOpPhi:
-        if (i != instructions.cbegin()) {
-          switch (prev(i)->opcode()) {
-            case SpvOpLabel:
-            case SpvOpPhi:
-            case SpvOpLine:
-              break;
-            default:
-              return _.diag(SPV_ERROR_INVALID_DATA, &(*i))
-                     << "OpPhi must appear before all non-OpPhi instructions "
-                     << "(except for OpLine, which can be mixed with OpPhi).";
-          }
+  const auto& inst = instructions[idx];
+
+  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).";
         }
-        break;
-      case SpvOpLoopMerge:
-        if (next(i) != instructions.cend()) {
-          switch (next(i)->opcode()) {
-            case SpvOpBranch:
-            case SpvOpBranchConditional:
-              break;
-            default:
-              return _.diag(SPV_ERROR_INVALID_DATA, &(*i))
-                     << "OpLoopMerge must immediately precede either an "
-                     << "OpBranch or OpBranchConditional instruction. "
-                     << "OpLoopMerge must be the second-to-last instruction in "
-                     << "its block.";
-          }
+      }
+      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 SpvOpSelectionMerge:
-        if (next(i) != instructions.cend()) {
-          switch (next(i)->opcode()) {
-            case SpvOpBranchConditional:
-            case SpvOpSwitch:
-              break;
-            default:
-              return _.diag(SPV_ERROR_INVALID_DATA, &(*i))
-                     << "OpSelectionMerge must immediately precede either an "
-                     << "OpBranchConditional or OpSwitch instruction. "
-                     << "OpSelectionMerge 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.";
         }
-      default:
-        break;
-    }
+      }
+    default:
+      break;
   }
 
   return SPV_SUCCESS;
diff --git a/source/val/validate_id.cpp b/source/val/validate_id.cpp
index baa8609c..af690518 100644
--- a/source/val/validate_id.cpp
+++ b/source/val/validate_id.cpp
@@ -1480,17 +1480,16 @@ bool idUsage::isValid(const spv_instruction_t* inst) {
 
 }  // namespace
 
-spv_result_t UpdateIdUse(ValidationState_t& _) {
-  for (const auto& inst : _.ordered_instructions()) {
-    for (auto& operand : inst.operands()) {
-      const spv_operand_type_t& type = operand.type;
-      const uint32_t operand_id = inst.word(operand.offset);
-      if (spvIsIdType(type) && type != SPV_OPERAND_TYPE_RESULT_ID) {
-        if (auto def = _.FindDef(operand_id))
-          def->RegisterUse(&inst, operand.offset);
-      }
+spv_result_t UpdateIdUse(ValidationState_t& _, const Instruction* inst) {
+  for (auto& operand : inst->operands()) {
+    const spv_operand_type_t& type = operand.type;
+    const uint32_t operand_id = inst->word(operand.offset);
+    if (spvIsIdType(type) && type != SPV_OPERAND_TYPE_RESULT_ID) {
+      if (auto def = _.FindDef(operand_id))
+        def->RegisterUse(inst, operand.offset);
     }
   }
+
   return SPV_SUCCESS;
 }
 
diff --git a/source/val/validate_memory.cpp b/source/val/validate_memory.cpp
index 1a503fd6..7f0f5788 100644
--- a/source/val/validate_memory.cpp
+++ b/source/val/validate_memory.cpp
@@ -561,36 +561,35 @@ spv_result_t ValidateAccessChain(ValidationState_t& _,
 
 }  // namespace
 
-spv_result_t ValidateMemoryInstructions(ValidationState_t& _) {
-  for (auto& inst : _.ordered_instructions()) {
-    switch (inst.opcode()) {
-      case SpvOpVariable:
-        if (auto error = ValidateVariable(_, inst)) return error;
-        break;
-      case SpvOpLoad:
-        if (auto error = ValidateLoad(_, inst)) return error;
-        break;
-      case SpvOpStore:
-        if (auto error = ValidateStore(_, inst)) return error;
-        break;
-      case SpvOpCopyMemory:
-        if (auto error = ValidateCopyMemory(_, inst)) return error;
-        break;
-      case SpvOpCopyMemorySized:
-        if (auto error = ValidateCopyMemorySized(_, inst)) return error;
-        break;
-      case SpvOpAccessChain:
-      case SpvOpInBoundsAccessChain:
-      case SpvOpPtrAccessChain:
-      case SpvOpInBoundsPtrAccessChain:
-        if (auto error = ValidateAccessChain(_, inst)) return error;
-        break;
-      case SpvOpImageTexelPointer:
-      case SpvOpArrayLength:
-      case SpvOpGenericPtrMemSemantics:
-      default:
-        break;
-    }
+spv_result_t ValidateMemoryInstructions(ValidationState_t& _,
+                                        const Instruction* inst) {
+  switch (inst->opcode()) {
+    case SpvOpVariable:
+      if (auto error = ValidateVariable(_, *inst)) return error;
+      break;
+    case SpvOpLoad:
+      if (auto error = ValidateLoad(_, *inst)) return error;
+      break;
+    case SpvOpStore:
+      if (auto error = ValidateStore(_, *inst)) return error;
+      break;
+    case SpvOpCopyMemory:
+      if (auto error = ValidateCopyMemory(_, *inst)) return error;
+      break;
+    case SpvOpCopyMemorySized:
+      if (auto error = ValidateCopyMemorySized(_, *inst)) return error;
+      break;
+    case SpvOpAccessChain:
+    case SpvOpInBoundsAccessChain:
+    case SpvOpPtrAccessChain:
+    case SpvOpInBoundsPtrAccessChain:
+      if (auto error = ValidateAccessChain(_, *inst)) return error;
+      break;
+    case SpvOpImageTexelPointer:
+    case SpvOpArrayLength:
+    case SpvOpGenericPtrMemSemantics:
+    default:
+      break;
   }
 
   return SPV_SUCCESS;