From: Ehsan Nasiri Date: Tue, 22 Nov 2016 23:06:55 +0000 (-0500) Subject: Validation for OpSampledImage instruction. X-Git-Tag: upstream/2018.6~1005 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=f72189c249ba143c6a89a4cf1e7d53337b2ddd40;p=platform%2Fupstream%2FSPIRV-Tools.git 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 --- 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 ValidationState_t::getSampledImageConsumers( + uint32_t sampled_image_id) const { + std::vector 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 --- 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 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 forward_pointer_ids_; + /// Stores a vector of instructions that use the result of a given + /// OpSampledImage instruction. + std::unordered_map> sampled_image_consumers_; + /// A map of operand IDs and their names defined by the OpName instruction std::unordered_map operand_names_; diff --git a/source/validate_id.cpp b/source/validate_id.cpp index 723d406..a486362 100644 --- a/source/validate_id.cpp +++ b/source/validate_id.cpp @@ -734,6 +734,54 @@ bool idUsage::isValid(const spv_instruction_t* inst, } template <> +bool idUsage::isValid(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 are consumed. + // * Result 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 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 are consumed. OpSampledImage Result " + "Type '" + << resultID << "' has a consumer in a different basic " + "block. The consumer instruction 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 from OpSampledImage instruction must not appear as " + "operands of Op" + << spvOpcodeString(static_cast(consumer_opcode)) << "." + << " Found result '" << resultID << "' as an operand of '" + << consumer_id << "'."; + return false; + } + } + } + return true; +} + +template <> bool idUsage::isValid(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 --- 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 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 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 are consumed. OpSampledImage Result " + "Type '23' has a consumer in a different basic block. The " + "consumer instruction is '25'.")); +} + +// Invalid: OpSampledImage result 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 from OpSampledImage instruction must not " + "appear as operands of OpSelect. Found result " + "'23' as an operand of '24'.")); +} + +// Invalid: OpSampledImage result 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 from OpSampledImage instruction must not " + "appear as operands of OpPhi. Found result '23' " + "as an operand of '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