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

diff --git a/CHANGES b/CHANGES
index e348488..417d62e 100644 (file)
--- 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 (file)
--- 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<void(uint32_t*)>& 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<void(const uint32_t*)>& 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 (file)
--- 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<uint32_t> ids;
+  inst.ForEachInId([&ids](const uint32_t* idptr) { ids.push_back(*idptr); });
+  EXPECT_THAT(ids, Eq(std::vector<uint32_t>{102, 103, 104, 105}));
+
+  ids.clear();
+  inst.ForEachInId([&ids](uint32_t* idptr) { ids.push_back(*idptr); });
+  EXPECT_THAT(ids, Eq(std::vector<uint32_t>{102, 103, 104, 105}));
+}
+
+TEST(InstructionTest, ForInIdNonstandardIdTypes) {
+  Instruction inst(kSampleControlBarrierInstruction);
+
+  std::vector<uint32_t> ids;
+  inst.ForEachInId([&ids](const uint32_t* idptr) { ids.push_back(*idptr); });
+  EXPECT_THAT(ids, Eq(std::vector<uint32_t>{100, 101, 102}));
+
+  ids.clear();
+  inst.ForEachInId([&ids](uint32_t* idptr) { ids.push_back(*idptr); });
+  EXPECT_THAT(ids, Eq(std::vector<uint32_t>{100, 101, 102}));
+}
+
 }  // anonymous namespace