From 35a06958441b68a74c9a817b33bba110b385d5f4 Mon Sep 17 00:00:00 2001 From: David Neto Date: Tue, 4 Jul 2017 16:24:46 -0400 Subject: [PATCH] Include memory and semantics IDs when iterating over inbound IDs Fixes Instruction::ForEachInId so it covers SPV_OPERAND_TYPE_MEMORY_SEMANTICS_ID and SPV_OPERAND_TYPE_SCOPE_ID. Future proof a bit by using the common spvIsIdType routine. Fixes https://github.com/KhronosGroup/SPIRV-Tools/issues/697 --- CHANGES | 2 ++ source/opt/instruction.h | 24 +++++++++++--- test/opt/instruction_test.cpp | 73 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 95 insertions(+), 4 deletions(-) diff --git a/CHANGES b/CHANGES index e348488..417d62e 100644 --- a/CHANGES +++ b/CHANGES @@ -41,6 +41,8 @@ v2016.7-dev 2017-01-06 binary vector when all passes succeded without changes. #629: The inline-entry-points-all optimization could generate invalidly structured code when the inlined function had early returns. + #697: Optimizer's Instruction::ForEachInId method was skipping semantics-id + and scope-id. v2016.6 2016-12-13 - Published the C++ interface for assembling, disassembling, validation, and diff --git a/source/opt/instruction.h b/source/opt/instruction.h index 0143f7c..89c9da0 100644 --- a/source/opt/instruction.h +++ b/source/opt/instruction.h @@ -247,14 +247,30 @@ inline void Instruction::ForEachInst( } inline void Instruction::ForEachInId(const std::function& f) { - for (auto& opnd : operands_) - if (opnd.type == SPV_OPERAND_TYPE_ID) f(&opnd.words[0]); + for (auto& opnd : operands_) { + switch (opnd.type) { + case SPV_OPERAND_TYPE_RESULT_ID: + case SPV_OPERAND_TYPE_TYPE_ID: + break; + default: + if (spvIsIdType(opnd.type)) f(&opnd.words[0]); + break; + } + } } inline void Instruction::ForEachInId( const std::function& f) const { - for (const auto& opnd : operands_) - if (opnd.type == SPV_OPERAND_TYPE_ID) f(&opnd.words[0]); + for (const auto& opnd : operands_) { + switch (opnd.type) { + case SPV_OPERAND_TYPE_RESULT_ID: + case SPV_OPERAND_TYPE_TYPE_ID: + break; + default: + if (spvIsIdType(opnd.type)) f(&opnd.words[0]); + break; + } + } } inline bool Instruction::HasLabels() const { diff --git a/test/opt/instruction_test.cpp b/test/opt/instruction_test.cpp index 608229f..2db4ed2 100644 --- a/test/opt/instruction_test.cpp +++ b/test/opt/instruction_test.cpp @@ -69,6 +69,55 @@ spv_parsed_instruction_t kSampleParsedInstruction = {kSampleInstructionWords, 44, // result id kSampleParsedOperands, 3}; + +// The words for an OpAccessChain instruction. +uint32_t kSampleAccessChainInstructionWords[] = { + (7 << 16) | uint32_t(SpvOpAccessChain), 100, 101, 102, 103, 104, 105}; + +// The operands that would be parsed from kSampleAccessChainInstructionWords. +spv_parsed_operand_t kSampleAccessChainOperands[] = { + {1, 1, SPV_OPERAND_TYPE_RESULT_ID, SPV_NUMBER_NONE, 0}, + {2, 1, SPV_OPERAND_TYPE_TYPE_ID, SPV_NUMBER_NONE, 0}, + {3, 1, SPV_OPERAND_TYPE_ID, SPV_NUMBER_NONE, 0}, + {4, 1, SPV_OPERAND_TYPE_ID, SPV_NUMBER_NONE, 0}, + {5, 1, SPV_OPERAND_TYPE_ID, SPV_NUMBER_NONE, 0}, + {6, 1, SPV_OPERAND_TYPE_ID, SPV_NUMBER_NONE, 0}, +}; + +// A valid parse of kSampleAccessChainInstructionWords +spv_parsed_instruction_t kSampleAccessChainInstruction = { + kSampleAccessChainInstructionWords, + uint16_t(7), + uint16_t(SpvOpAccessChain), + SPV_EXT_INST_TYPE_NONE, + 100, // type id + 101, // result id + kSampleAccessChainOperands, + 6}; + +// The words for an OpControlBarrier instruction. +uint32_t kSampleControlBarrierInstructionWords[] = { + (4 << 16) | uint32_t(SpvOpControlBarrier), 100, 101, 102}; + +// The operands that would be parsed from kSampleControlBarrierInstructionWords. +spv_parsed_operand_t kSampleControlBarrierOperands[] = { + {1, 1, SPV_OPERAND_TYPE_SCOPE_ID, SPV_NUMBER_NONE, 0}, // Execution + {2, 1, SPV_OPERAND_TYPE_SCOPE_ID, SPV_NUMBER_NONE, 0}, // Memory + {3, 1, SPV_OPERAND_TYPE_MEMORY_SEMANTICS_ID, SPV_NUMBER_NONE, + 0}, // Semantics +}; + +// A valid parse of kSampleControlBarrierInstructionWords +spv_parsed_instruction_t kSampleControlBarrierInstruction = { + kSampleControlBarrierInstructionWords, + uint16_t(4), + uint16_t(SpvOpControlBarrier), + SPV_EXT_INST_TYPE_NONE, + 0, // type id + 0, // result id + kSampleControlBarrierOperands, + 3}; + TEST(InstructionTest, CreateWithOpcodeAndOperands) { Instruction inst(kSampleParsedInstruction); EXPECT_EQ(SpvOpTypeInt, inst.opcode()); @@ -148,4 +197,28 @@ TEST(InstructionTest, OperandIterators) { EXPECT_EQ(SPV_OPERAND_TYPE_TYPE_ID, (*(inst.cbegin() + 2)).type); } +TEST(InstructionTest, ForInIdStandardIdTypes) { + Instruction inst(kSampleAccessChainInstruction); + + std::vector ids; + inst.ForEachInId([&ids](const uint32_t* idptr) { ids.push_back(*idptr); }); + EXPECT_THAT(ids, Eq(std::vector{102, 103, 104, 105})); + + ids.clear(); + inst.ForEachInId([&ids](uint32_t* idptr) { ids.push_back(*idptr); }); + EXPECT_THAT(ids, Eq(std::vector{102, 103, 104, 105})); +} + +TEST(InstructionTest, ForInIdNonstandardIdTypes) { + Instruction inst(kSampleControlBarrierInstruction); + + std::vector ids; + inst.ForEachInId([&ids](const uint32_t* idptr) { ids.push_back(*idptr); }); + EXPECT_THAT(ids, Eq(std::vector{100, 101, 102})); + + ids.clear(); + inst.ForEachInId([&ids](uint32_t* idptr) { ids.push_back(*idptr); }); + EXPECT_THAT(ids, Eq(std::vector{100, 101, 102})); +} + } // anonymous namespace -- 2.7.4