Refactor where opcodes are validated

* Replaced uses in opcode validation of current_function()
* Added non-const accessor to function lookup in ValidationState_t
* Updated a couple bad tests due to check reordering

9 files changed, 114 insertions, 76 deletions

diff --git a/source/val/validate.cpp b/source/val/validate.cpp
index 7fd9fe85..d642ec0d 100644
--- a/source/val/validate.cpp
+++ b/source/val/validate.cpp
@@ -242,9 +242,7 @@ spv_result_t ValidateBinaryUsingContextAndValidationState(
     return error;
   }
 
-  for (size_t i = 0; i < vstate->ordered_instructions().size(); ++i) {
-    auto& instruction = vstate->ordered_instructions()[i];
-
+  for (auto& instruction : vstate->ordered_instructions()) {
     {
       // In order to do this work outside of Process Instruction we need to be
       // able to, briefly, de-const the instruction.
@@ -292,28 +290,7 @@ spv_result_t ValidateBinaryUsingContextAndValidationState(
     if (auto error = ModuleLayoutPass(*vstate, &instruction)) return error;
     if (auto error = CfgPass(*vstate, &instruction)) return error;
     if (auto error = InstructionPass(*vstate, &instruction)) return error;
-    if (auto error = TypeUniquePass(*vstate, &instruction)) return error;
-    if (auto error = ArithmeticsPass(*vstate, &instruction)) return error;
-    if (auto error = CompositesPass(*vstate, &instruction)) return error;
-    if (auto error = ConversionPass(*vstate, &instruction)) return error;
-    if (auto error = DerivativesPass(*vstate, &instruction)) return error;
-    if (auto error = LogicalsPass(*vstate, &instruction)) return error;
-    if (auto error = BitwisePass(*vstate, &instruction)) return error;
-    if (auto error = ExtInstPass(*vstate, &instruction)) return error;
-    if (auto error = ImagePass(*vstate, &instruction)) return error;
-    if (auto error = AtomicsPass(*vstate, &instruction)) return error;
-    if (auto error = BarriersPass(*vstate, &instruction)) return error;
-    if (auto error = PrimitivesPass(*vstate, &instruction)) return error;
-    if (auto error = LiteralsPass(*vstate, &instruction)) return error;
-    if (auto error = NonUniformPass(*vstate, &instruction)) return error;
-
     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())
@@ -337,6 +314,31 @@ spv_result_t ValidateBinaryUsingContextAndValidationState(
   // for() above as it loops over all ordered_instructions internally.
   if (auto error = ValidateBuiltIns(*vstate)) return error;
 
+  // Validate individual opcodes.
+  for (size_t i = 0; i < vstate->ordered_instructions().size(); ++i) {
+    auto& instruction = vstate->ordered_instructions()[i];
+    if (auto error = TypeUniquePass(*vstate, &instruction)) return error;
+    if (auto error = ArithmeticsPass(*vstate, &instruction)) return error;
+    if (auto error = CompositesPass(*vstate, &instruction)) return error;
+    if (auto error = ConversionPass(*vstate, &instruction)) return error;
+    if (auto error = DerivativesPass(*vstate, &instruction)) return error;
+    if (auto error = LogicalsPass(*vstate, &instruction)) return error;
+    if (auto error = BitwisePass(*vstate, &instruction)) return error;
+    if (auto error = ExtInstPass(*vstate, &instruction)) return error;
+    if (auto error = ImagePass(*vstate, &instruction)) return error;
+    if (auto error = AtomicsPass(*vstate, &instruction)) return error;
+    if (auto error = BarriersPass(*vstate, &instruction)) return error;
+    if (auto error = PrimitivesPass(*vstate, &instruction)) return error;
+    if (auto error = LiteralsPass(*vstate, &instruction)) return error;
+    if (auto error = NonUniformPass(*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;
+  }
+
   // NOTE: Copy each instruction for easier processing
   std::vector<spv_instruction_t> instructions;
   // Expect average instruction length to be a bit over 2 words.
diff --git a/source/val/validate_barriers.cpp b/source/val/validate_barriers.cpp
index 895df363..0771f2d2 100644
--- a/source/val/validate_barriers.cpp
+++ b/source/val/validate_barriers.cpp
@@ -58,8 +58,9 @@ spv_result_t ValidateExecutionScope(ValidationState_t& _,
 
     if (_.context()->target_env != SPV_ENV_VULKAN_1_0 &&
         value != SpvScopeSubgroup) {
-      _.current_function().RegisterExecutionModelLimitation(
-          [](SpvExecutionModel model, std::string* message) {
+      _.function(inst->function()->id())
+          ->RegisterExecutionModelLimitation([](SpvExecutionModel model,
+                                                std::string* message) {
             if (model == SpvExecutionModelFragment ||
                 model == SpvExecutionModelVertex ||
                 model == SpvExecutionModelGeometry ||
@@ -200,21 +201,22 @@ spv_result_t BarriersPass(ValidationState_t& _, const Instruction* inst) {
     case SpvOpControlBarrier: {
       if (spvVersionForTargetEnv(_.context()->target_env) <
           SPV_SPIRV_VERSION_WORD(1, 3)) {
-        _.current_function().RegisterExecutionModelLimitation(
-            [](SpvExecutionModel model, std::string* message) {
-              if (model != SpvExecutionModelTessellationControl &&
-                  model != SpvExecutionModelGLCompute &&
-                  model != SpvExecutionModelKernel) {
-                if (message) {
-                  *message =
-                      "OpControlBarrier requires one of the following "
-                      "Execution "
-                      "Models: TessellationControl, GLCompute or Kernel";
-                }
-                return false;
-              }
-              return true;
-            });
+        _.function(inst->function()->id())
+            ->RegisterExecutionModelLimitation(
+                [](SpvExecutionModel model, std::string* message) {
+                  if (model != SpvExecutionModelTessellationControl &&
+                      model != SpvExecutionModelGLCompute &&
+                      model != SpvExecutionModelKernel) {
+                    if (message) {
+                      *message =
+                          "OpControlBarrier requires one of the following "
+                          "Execution "
+                          "Models: TessellationControl, GLCompute or Kernel";
+                    }
+                    return false;
+                  }
+                  return true;
+                });
       }
 
       const uint32_t execution_scope = inst->word(1);
diff --git a/source/val/validate_derivatives.cpp b/source/val/validate_derivatives.cpp
index 951e2142..0e0dbbe3 100644
--- a/source/val/validate_derivatives.cpp
+++ b/source/val/validate_derivatives.cpp
@@ -54,10 +54,12 @@ spv_result_t DerivativesPass(ValidationState_t& _, const Instruction* inst) {
                << spvOpcodeString(opcode);
       }
 
-      _.current_function().RegisterExecutionModelLimitation(
-          SpvExecutionModelFragment, std::string(
-              "Derivative instructions require Fragment execution model: ") +
-          spvOpcodeString(opcode));
+      _.function(inst->function()->id())
+          ->RegisterExecutionModelLimitation(
+              SpvExecutionModelFragment,
+              std::string("Derivative instructions require Fragment execution "
+                          "model: ") +
+                  spvOpcodeString(opcode));
       break;
     }
 
diff --git a/source/val/validate_ext_inst.cpp b/source/val/validate_ext_inst.cpp
index 24565719..eb342709 100644
--- a/source/val/validate_ext_inst.cpp
+++ b/source/val/validate_ext_inst.cpp
@@ -744,10 +744,11 @@ spv_result_t ExtInstPass(ValidationState_t& _, const Instruction* inst) {
           }
         }
 
-        _.current_function().RegisterExecutionModelLimitation(
-            SpvExecutionModelFragment,
-            ext_inst_name() +
-                std::string(" requires Fragment execution model"));
+        _.function(inst->function()->id())
+            ->RegisterExecutionModelLimitation(
+                SpvExecutionModelFragment,
+                ext_inst_name() +
+                    std::string(" requires Fragment execution model"));
         break;
       }
 
diff --git a/source/val/validate_image.cpp b/source/val/validate_image.cpp
index 3f3f623a..0d2dd00c 100644
--- a/source/val/validate_image.cpp
+++ b/source/val/validate_image.cpp
@@ -1095,10 +1095,11 @@ spv_result_t ValidateImageRead(ValidationState_t& _, const Instruction* inst) {
              << "Image Dim SubpassData cannot be used with ImageSparseRead";
     }
 
-    _.current_function().RegisterExecutionModelLimitation(
-        SpvExecutionModelFragment,
-        std::string("Dim SubpassData requires Fragment execution model: ") +
-            spvOpcodeString(opcode));
+    _.function(inst->function()->id())
+        ->RegisterExecutionModelLimitation(
+            SpvExecutionModelFragment,
+            std::string("Dim SubpassData requires Fragment execution model: ") +
+                spvOpcodeString(opcode));
   }
 
   if (_.GetIdOpcode(info.sampled_type) != SpvOpTypeVoid) {
@@ -1387,9 +1388,10 @@ spv_result_t ValidateImageQueryFormatOrOrder(ValidationState_t& _,
 
 spv_result_t ValidateImageQueryLod(ValidationState_t& _,
                                    const Instruction* inst) {
-  _.current_function().RegisterExecutionModelLimitation(
-      SpvExecutionModelFragment,
-      "OpImageQueryLod requires Fragment execution model");
+  _.function(inst->function()->id())
+      ->RegisterExecutionModelLimitation(
+          SpvExecutionModelFragment,
+          "OpImageQueryLod requires Fragment execution model");
 
   const uint32_t result_type = inst->type_id();
   if (!_.IsFloatVectorType(result_type)) {
@@ -1512,9 +1514,10 @@ spv_result_t ValidateImageSparseTexelsResident(ValidationState_t& _,
 spv_result_t ImagePass(ValidationState_t& _, const Instruction* inst) {
   const SpvOp opcode = inst->opcode();
   if (IsImplicitLod(opcode)) {
-    _.current_function().RegisterExecutionModelLimitation(
-        SpvExecutionModelFragment,
-        "ImplicitLod instructions require Fragment execution model");
+    _.function(inst->function()->id())
+        ->RegisterExecutionModelLimitation(
+            SpvExecutionModelFragment,
+            "ImplicitLod instructions require Fragment execution model");
   }
 
   switch (opcode) {
diff --git a/source/val/validate_primitives.cpp b/source/val/validate_primitives.cpp
index 3616cb99..7d11f2e7 100644
--- a/source/val/validate_primitives.cpp
+++ b/source/val/validate_primitives.cpp
@@ -35,10 +35,11 @@ spv_result_t PrimitivesPass(ValidationState_t& _, const Instruction* inst) {
     case SpvOpEndPrimitive:
     case SpvOpEmitStreamVertex:
     case SpvOpEndStreamPrimitive:
-      _.current_function().RegisterExecutionModelLimitation(
-          SpvExecutionModelGeometry,
-          std::string(spvOpcodeString(opcode)) +
-              " instructions require Geometry execution model");
+      _.function(inst->function()->id())
+          ->RegisterExecutionModelLimitation(
+              SpvExecutionModelGeometry,
+              std::string(spvOpcodeString(opcode)) +
+                  " instructions require Geometry execution model");
       break;
     default:
       break;
diff --git a/source/val/validation_state.cpp b/source/val/validation_state.cpp
index e0c9e5ba..c2b08e8a 100644
--- a/source/val/validation_state.cpp
+++ b/source/val/validation_state.cpp
@@ -321,6 +321,12 @@ const Function* ValidationState_t::function(uint32_t id) const {
   return it->second;
 }
 
+Function* ValidationState_t::function(uint32_t id) {
+  auto it = id_to_function_.find(id);
+  if (it == id_to_function_.end()) return nullptr;
+  return it->second;
+}
+
 bool ValidationState_t::in_function_body() const { return in_function_; }
 
 bool ValidationState_t::in_block() const {
diff --git a/source/val/validation_state.h b/source/val/validation_state.h
index d93fe1e9..76998591 100644
--- a/source/val/validation_state.h
+++ b/source/val/validation_state.h
@@ -184,6 +184,7 @@ class ValidationState_t {
 
   /// Returns function state with the given id, or nullptr if no such function.
   const Function* function(uint32_t id) const;
+  Function* function(uint32_t id);
 
   /// Returns true if the called after a function instruction but before the
   /// function end instruction
diff --git a/test/val/val_image_test.cpp b/test/val/val_image_test.cpp
index d281327e..03f3eeb9 100644
--- a/test/val/val_image_test.cpp
+++ b/test/val/val_image_test.cpp
@@ -339,8 +339,8 @@ OpFunctionEnd)";
   return ss.str();
 }
 
-std::string GetShaderHeader(
-    const std::string& capabilities_and_extensions = "") {
+std::string GetShaderHeader(const std::string& capabilities_and_extensions = "",
+                            bool include_entry_point = true) {
   std::ostringstream ss;
   ss << R"(
 OpCapability Shader
@@ -348,10 +348,18 @@ OpCapability Int64
 )";
 
   ss << capabilities_and_extensions;
+  if (!include_entry_point) {
+    ss << "OpCapability Linkage";
+  }
 
   ss << R"(
 OpMemoryModel Logical GLSL450
-OpEntryPoint Fragment %main "main"
+)";
+
+  if (include_entry_point) {
+    ss << "OpEntryPoint Fragment %main \"main\"";
+  }
+  ss << R"(
 %void = OpTypeVoid
 %func = OpTypeFunction %void
 %bool = OpTypeBool
@@ -365,7 +373,7 @@ OpEntryPoint Fragment %main "main"
 }
 
 TEST_F(ValidateImage, TypeImageWrongSampledType) {
-  const std::string code = GetShaderHeader() + R"(
+  const std::string code = GetShaderHeader("", false) + R"(
 %img_type = OpTypeImage %bool 2D 0 0 0 1 Unknown
 )";
 
@@ -380,6 +388,11 @@ TEST_F(ValidateImage, TypeImageWrongSampledType) {
 TEST_F(ValidateImage, TypeImageVoidSampledTypeVulkan) {
   const std::string code = GetShaderHeader() + R"(
 %img_type = OpTypeImage %void 2D 0 0 0 1 Unknown
+%void_func = OpTypeFunction %void
+%main = OpFunction %void None %void_func
+%main_lab = OpLabel
+OpReturn
+OpFunctionEnd
 )";
 
   const spv_target_env env = SPV_ENV_VULKAN_1_0;
@@ -393,6 +406,11 @@ TEST_F(ValidateImage, TypeImageVoidSampledTypeVulkan) {
 TEST_F(ValidateImage, TypeImageU64SampledTypeVulkan) {
   const std::string code = GetShaderHeader() + R"(
 %img_type = OpTypeImage %u64 2D 0 0 0 1 Unknown
+%void_func = OpTypeFunction %void
+%main = OpFunction %void None %void_func
+%main_lab = OpLabel
+OpReturn
+OpFunctionEnd
 )";
 
   const spv_target_env env = SPV_ENV_VULKAN_1_0;
@@ -404,7 +422,7 @@ TEST_F(ValidateImage, TypeImageU64SampledTypeVulkan) {
 }
 
 TEST_F(ValidateImage, TypeImageWrongDepth) {
-  const std::string code = GetShaderHeader() + R"(
+  const std::string code = GetShaderHeader("", false) + R"(
 %img_type = OpTypeImage %f32 2D 3 0 0 1 Unknown
 )";
 
@@ -415,7 +433,7 @@ TEST_F(ValidateImage, TypeImageWrongDepth) {
 }
 
 TEST_F(ValidateImage, TypeImageWrongArrayed) {
-  const std::string code = GetShaderHeader() + R"(
+  const std::string code = GetShaderHeader("", false) + R"(
 %img_type = OpTypeImage %f32 2D 0 2 0 1 Unknown
 )";
 
@@ -426,7 +444,7 @@ TEST_F(ValidateImage, TypeImageWrongArrayed) {
 }
 
 TEST_F(ValidateImage, TypeImageWrongMS) {
-  const std::string code = GetShaderHeader() + R"(
+  const std::string code = GetShaderHeader("", false) + R"(
 %img_type = OpTypeImage %f32 2D 0 0 2 1 Unknown
 )";
 
@@ -437,7 +455,7 @@ TEST_F(ValidateImage, TypeImageWrongMS) {
 }
 
 TEST_F(ValidateImage, TypeImageWrongSampled) {
-  const std::string code = GetShaderHeader() + R"(
+  const std::string code = GetShaderHeader("", false) + R"(
 %img_type = OpTypeImage %f32 2D 0 0 0 3 Unknown
 )";
 
@@ -448,8 +466,9 @@ TEST_F(ValidateImage, TypeImageWrongSampled) {
 }
 
 TEST_F(ValidateImage, TypeImageWrongSampledForSubpassData) {
-  const std::string code = GetShaderHeader("OpCapability InputAttachment\n") +
-                           R"(
+  const std::string code =
+      GetShaderHeader("OpCapability InputAttachment\n", false) +
+      R"(
 %img_type = OpTypeImage %f32 SubpassData 0 0 0 1 Unknown
 )";
 
@@ -460,8 +479,9 @@ TEST_F(ValidateImage, TypeImageWrongSampledForSubpassData) {
 }
 
 TEST_F(ValidateImage, TypeImageWrongFormatForSubpassData) {
-  const std::string code = GetShaderHeader("OpCapability InputAttachment\n") +
-                           R"(
+  const std::string code =
+      GetShaderHeader("OpCapability InputAttachment\n", false) +
+      R"(
 %img_type = OpTypeImage %f32 SubpassData 0 0 0 2 Rgba32f
 )";
 
@@ -472,7 +492,7 @@ TEST_F(ValidateImage, TypeImageWrongFormatForSubpassData) {
 }
 
 TEST_F(ValidateImage, TypeSampledImageNotImage) {
-  const std::string code = GetShaderHeader() + R"(
+  const std::string code = GetShaderHeader("", false) + R"(
 %simg_type = OpTypeSampledImage %f32
 )";