Validation for OpSampledImage instruction.

This change implements the validation for usages of OpSampledImage
instruction as described in the Data Rules section of the Universal
Validation Rules of the SPIR-V Spec.

diff --git a/source/val/validation_state.cpp b/source/val/validation_state.cpp
index 05720e1..16622ff 100644 (file)
--- a/source/val/validation_state.cpp
+++ b/source/val/validation_state.cpp
@@ -394,5 +394,35 @@ void ValidationState_t::RegisterInstruction(
   if (id) {
     all_definitions_.insert(make_pair(id, &ordered_instructions_.back()));
   }
+
+  // If the instruction is using an OpTypeSampledImage as an operand, it should
+  // be recorded. The validator will ensure that all usages of an
+  // OpTypeSampledImage and its definition are in the same basic block.
+  for (uint16_t i = 0; i < inst.num_operands; ++i) {
+    const spv_parsed_operand_t& operand = inst.operands[i];
+    if (SPV_OPERAND_TYPE_ID == operand.type) {
+      const uint32_t operand_word = inst.words[operand.offset];
+      Instruction* operand_inst = FindDef(operand_word);
+      if (operand_inst && SpvOpSampledImage == operand_inst->opcode()) {
+        RegisterSampledImageConsumer(operand_word, inst.result_id);
+      }
+    }
+  }
 }
+
+std::vector<uint32_t> ValidationState_t::getSampledImageConsumers(
+    uint32_t sampled_image_id) const {
+  std::vector<uint32_t> result;
+  auto iter = sampled_image_consumers_.find(sampled_image_id);
+  if (iter != sampled_image_consumers_.end()) {
+    result = iter->second;
+  }
+  return result;
+}
+
+void ValidationState_t::RegisterSampledImageConsumer(uint32_t sampled_image_id,
+                                                     uint32_t consumer_id) {
+  sampled_image_consumers_[sampled_image_id].push_back(consumer_id);
+}
+
 }  /// namespace libspirv

diff --git a/source/val/validation_state.h b/source/val/validation_state.h
index cc00b84..0647e7e 100644 (file)
--- a/source/val/validation_state.h
+++ b/source/val/validation_state.h
@@ -179,6 +179,14 @@ class ValidationState_t {
     return all_definitions_;
   }
 
+  /// Returns a vector containing the Ids of instructions that consume the given
+  /// SampledImage id.
+  std::vector<uint32_t> getSampledImageConsumers(uint32_t id) const;
+
+  /// Records cons_id as a consumer of sampled_image_id.
+  void RegisterSampledImageConsumer(uint32_t sampled_image_id,
+                                    uint32_t cons_id);
+
  private:
   ValidationState_t(const ValidationState_t&);
 
@@ -193,6 +201,10 @@ class ValidationState_t {
   /// IDs that have been declared as forward pointers.
   std::unordered_set<uint32_t> forward_pointer_ids_;
 
+  /// Stores a vector of instructions that use the result of a given
+  /// OpSampledImage instruction.
+  std::unordered_map<uint32_t, std::vector<uint32_t>> sampled_image_consumers_;
+
   /// A map of operand IDs and their names defined by the OpName instruction
   std::unordered_map<uint32_t, std::string> operand_names_;
 

diff --git a/source/validate_id.cpp b/source/validate_id.cpp
index 723d406..a486362 100644 (file)
--- a/source/validate_id.cpp
+++ b/source/validate_id.cpp
@@ -734,6 +734,54 @@ bool idUsage::isValid<SpvOpSpecConstantFalse>(const spv_instruction_t* inst,
 }
 
 template <>
+bool idUsage::isValid<SpvOpSampledImage>(const spv_instruction_t* inst,
+                                         const spv_opcode_desc) {
+  auto resultTypeIndex = 2;
+  auto resultID = inst->words[resultTypeIndex];
+  auto sampledImageInstr = module_.FindDef(resultID);
+  // We need to validate 2 things:
+  // * All OpSampledImage instructions must be in the same block in which their
+  // Result <id> are consumed.
+  // * Result <id> from OpSampledImage instructions must not appear as operands
+  // to OpPhi instructions or OpSelect instructions, or any instructions other
+  // than the image lookup and query instructions specified to take an operand
+  // whose type is OpTypeSampledImage.
+  std::vector<uint32_t> consumers =
+      module_.getSampledImageConsumers(resultID);
+  if (!consumers.empty()) {
+    for (auto consumer_id : consumers) {
+      auto consumer_instr = module_.FindDef(consumer_id);
+      auto consumer_opcode = consumer_instr->opcode();
+      if (consumer_instr->block() != sampledImageInstr->block()) {
+        DIAG(resultTypeIndex)
+            << "All OpSampledImage instructions must be in the same block in "
+               "which their Result <id> are consumed. OpSampledImage Result "
+               "Type <id> '"
+            << resultID << "' has a consumer in a different basic "
+                           "block. The consumer instruction <id> is '"
+            << consumer_id << "'.";
+        return false;
+      }
+      // TODO: The following check is incomplete. We should also check that the
+      // Sampled Image is not used by instructions that should not take
+      // SampledImage as an argument. We could find the list of valid
+      // instructions by scanning for "Sampled Image" in the operand description
+      // field in the grammar file.
+      if (consumer_opcode == SpvOpPhi || consumer_opcode == SpvOpSelect) {
+        DIAG(resultTypeIndex)
+            << "Result <id> from OpSampledImage instruction must not appear as "
+               "operands of Op"
+            << spvOpcodeString(static_cast<SpvOp>(consumer_opcode)) << "."
+            << " Found result <id> '" << resultID << "' as an operand of <id> '"
+            << consumer_id << "'.";
+        return false;
+      }
+    }
+  }
+  return true;
+}
+
+template <>
 bool idUsage::isValid<SpvOpSpecConstantComposite>(const spv_instruction_t* inst,
                                                   const spv_opcode_desc) {
   // The result type must be a composite type.
@@ -2360,6 +2408,7 @@ bool idUsage::isValid(const spv_instruction_t* inst) {
     CASE(OpSpecConstantTrue)
     CASE(OpSpecConstantFalse)
     CASE(OpSpecConstantComposite)
+    CASE(OpSampledImage)
     TODO(OpSpecConstantOp)
     CASE(OpVariable)
     CASE(OpLoad)

diff --git a/test/val/val_id_test.cpp b/test/val/val_id_test.cpp
index 8fae465..5dafa08 100644 (file)
--- a/test/val/val_id_test.cpp
+++ b/test/val/val_id_test.cpp
@@ -68,6 +68,31 @@ string kOpenCLMemoryModel64 = R"(
      OpMemoryModel Physical64 OpenCL
 )";
 
+string sampledImageSetup = R"(
+                    %void = OpTypeVoid
+            %typeFuncVoid = OpTypeFunction %void
+                   %float = OpTypeFloat 32
+                 %v4float = OpTypeVector %float 4
+              %image_type = OpTypeImage %float 2D 0 0 0 1 Unknown
+%_ptr_UniformConstant_img = OpTypePointer UniformConstant %image_type
+                     %tex = OpVariable %_ptr_UniformConstant_img UniformConstant
+            %sampler_type = OpTypeSampler
+%_ptr_UniformConstant_sam = OpTypePointer UniformConstant %sampler_type
+                       %s = OpVariable %_ptr_UniformConstant_sam UniformConstant
+      %sampled_image_type = OpTypeSampledImage %image_type
+                 %v2float = OpTypeVector %float 2
+                 %float_1 = OpConstant %float 1
+                 %float_2 = OpConstant %float 2
+           %const_vec_1_1 = OpConstantComposite %v2float %float_1 %float_1
+           %const_vec_2_2 = OpConstantComposite %v2float %float_2 %float_2
+               %bool_type = OpTypeBool
+               %spec_true = OpSpecConstantTrue %bool_type
+                    %main = OpFunction %void None %typeFuncVoid
+                 %label_1 = OpLabel
+              %image_inst = OpLoad %image_type %tex
+            %sampler_inst = OpLoad %sampler_type %s
+)";
+
 // TODO: OpUndef
 
 TEST_F(ValidateIdWithMessage, OpName) {
@@ -1925,6 +1950,74 @@ TEST_F(ValidateIdWithMessage, OpFunctionCallArgumentTypeBad) {
   CompileSuccessfully(spirv.c_str());
   EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
 }
+
+// Valid: OpSampledImage result <id> is used in the same block by
+// OpImageSampleImplictLod
+TEST_F(ValidateIdWithMessage, OpSampledImageGood) {
+  string spirv = kGLSL450MemoryModel + sampledImageSetup + R"(
+%smpld_img = OpSampledImage %sampled_image_type %image_inst %sampler_inst
+%si_lod    = OpImageSampleImplicitLod %v4float %smpld_img %const_vec_1_1
+    OpReturn
+    OpFunctionEnd)";
+  CompileSuccessfully(spirv.c_str());
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
+}
+
+// Invalid: OpSampledImage result <id> is defined in one block and used in a
+// different block.
+TEST_F(ValidateIdWithMessage, OpSampledImageUsedInDifferentBlockBad) {
+  string spirv = kGLSL450MemoryModel + sampledImageSetup + R"(
+%smpld_img = OpSampledImage %sampled_image_type %image_inst %sampler_inst
+OpBranch %label_2
+%label_2 = OpLabel
+%si_lod  = OpImageSampleImplicitLod %v4float %smpld_img %const_vec_1_1
+OpReturn
+OpFunctionEnd)";
+  CompileSuccessfully(spirv.c_str());
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr("All OpSampledImage instructions must be in the same block in "
+                "which their Result <id> are consumed. OpSampledImage Result "
+                "Type <id> '23' has a consumer in a different basic block. The "
+                "consumer instruction <id> is '25'."));
+}
+
+// Invalid: OpSampledImage result <id> is used by OpSelect
+// Note: According to the Spec, OpSelect parameters must be either a scalar or a
+// vector. Therefore, OpTypeSampledImage is an illegal parameter for OpSelect.
+// However, the OpSelect validation does not catch this today. Therefore, it is
+// caught by the OpSampledImage validation. If the OpSelect validation code is
+// updated, the error message for this test may change.
+TEST_F(ValidateIdWithMessage, OpSampledImageUsedInOpSelectBad) {
+  string spirv = kGLSL450MemoryModel + sampledImageSetup + R"(
+%smpld_img  = OpSampledImage %sampled_image_type %image_inst %sampler_inst
+%select_img = OpSelect %sampled_image_type %spec_true %smpld_img %smpld_img
+OpReturn
+OpFunctionEnd)";
+  CompileSuccessfully(spirv.c_str());
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("Result <id> from OpSampledImage instruction must not "
+                        "appear as operands of OpSelect. Found result <id> "
+                        "'23' as an operand of <id> '24'."));
+}
+
+// Invalid: OpSampledImage result <id> is used by OpPhi
+TEST_F(ValidateIdWithMessage, OpSampledImageUsedInOpPhiBad) {
+  string spirv = kGLSL450MemoryModel + sampledImageSetup + R"(
+%smpld_img  = OpSampledImage %sampled_image_type %image_inst %sampler_inst
+%phi_result = OpPhi %sampled_image_type %smpld_img %label_1
+OpReturn
+OpFunctionEnd)";
+  CompileSuccessfully(spirv.c_str());
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions());
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("Result <id> from OpSampledImage instruction must not "
+                        "appear as operands of OpPhi. Found result <id> '23' "
+                        "as an operand of <id> '24'."));
+}
+
 #if 0
 TEST_F(ValidateIdWithMessage, OpFunctionCallArgumentCountBar) {
   const char *spirv = R"(
@@ -1950,7 +2043,6 @@ TEST_F(ValidateIdWithMessage, OpFunctionCallArgumentCountBar) {
 }
 #endif
 
-// TODO: OpSampledImage
 // TODO: The many things that changed with how images are used.
 // TODO: OpTextureSample
 // TODO: OpTextureSampleDref