spirv-diff: Leave undefined ids unpaired. (#5262)

If an id in one module is not defined by any instruction, don't bother matching it with an id in the other module, as this disturbs the reported id bound, resulting in spurious differences. Fixes #5260.
2024-10-18 11:10:05 +00:00 · 2023-06-09 12:00:46 -07:00 · 2023-06-09 12:00:46 -07:00 · ae1843b67c
commit ae1843b67c
parent 93c13345e1
10 changed files with 101 additions and 104 deletions
--- a/source/diff/diff.cpp
+++ b/source/diff/diff.cpp
@ -108,10 +108,6 @@ class IdMap {
    return inst_map_.find(from_inst) != inst_map_.end();
  }

-  // Map any ids in src and dst that have not been mapped to new ids in dst and
-  // src respectively.
-  void MapUnmatchedIds(IdMap& other_way);
-
  // Some instructions don't have result ids.  Those are mapped by pointer.
  void MapInsts(const opt::Instruction* from_inst,
                const opt::Instruction* to_inst) {
@ -124,6 +120,12 @@ class IdMap {

  uint32_t IdBound() const { return static_cast<uint32_t>(id_map_.size()); }

+  // Generate a fresh id in this mapping's domain.
+  uint32_t MakeFreshId() {
+    id_map_.push_back(0);
+    return static_cast<uint32_t>(id_map_.size()) - 1;
+  }
+
 private:
  // Given an id, returns the corresponding id in the other module, or 0 if not
  // matched yet.
@ -162,8 +164,11 @@ class SrcDstIdMap {
  }

  // Map any ids in src and dst that have not been mapped to new ids in dst and
-  // src respectively.
-  void MapUnmatchedIds();
+  // src respectively. Use src_insn_defined and dst_insn_defined to ignore ids
+  // that are simply never defined. (Since we assume the inputs are valid
+  // SPIR-V, this implies they are also never used.)
+  void MapUnmatchedIds(std::function<bool(uint32_t)> src_insn_defined,
+                       std::function<bool(uint32_t)> dst_insn_defined);

  // Some instructions don't have result ids.  Those are mapped by pointer.
  void MapInsts(const opt::Instruction* src_inst,
@ -213,6 +218,11 @@ struct IdInstructions {

  void MapIdToInstruction(uint32_t id, const opt::Instruction* inst);

+  // Return true if id is mapped to any instruction, false otherwise.
+  bool IsDefined(uint32_t id) {
+    return id < inst_map_.size() && inst_map_[id] != nullptr;
+  }
+
  void MapIdsToInstruction(
      opt::IteratorRange<opt::Module::const_inst_iterator> section);
  void MapIdsToInfos(
@ -567,36 +577,27 @@ class Differ {
  FunctionMap dst_funcs_;
 };

-void IdMap::MapUnmatchedIds(IdMap& other_way) {
-  const uint32_t src_id_bound = static_cast<uint32_t>(id_map_.size());
-  const uint32_t dst_id_bound = static_cast<uint32_t>(other_way.id_map_.size());
-
-  uint32_t next_src_id = src_id_bound;
-  uint32_t next_dst_id = dst_id_bound;
+void SrcDstIdMap::MapUnmatchedIds(
+    std::function<bool(uint32_t)> src_insn_defined,
+    std::function<bool(uint32_t)> dst_insn_defined) {
+  const uint32_t src_id_bound = static_cast<uint32_t>(src_to_dst_.IdBound());
+  const uint32_t dst_id_bound = static_cast<uint32_t>(dst_to_src_.IdBound());

  for (uint32_t src_id = 1; src_id < src_id_bound; ++src_id) {
-    if (!IsMapped(src_id)) {
-      MapIds(src_id, next_dst_id);
-
-      other_way.id_map_.push_back(0);
-      other_way.MapIds(next_dst_id++, src_id);
+    if (!src_to_dst_.IsMapped(src_id) && src_insn_defined(src_id)) {
+      uint32_t fresh_dst_id = dst_to_src_.MakeFreshId();
+      MapIds(src_id, fresh_dst_id);
    }
  }

  for (uint32_t dst_id = 1; dst_id < dst_id_bound; ++dst_id) {
-    if (!other_way.IsMapped(dst_id)) {
-      id_map_.push_back(0);
-      MapIds(next_src_id, dst_id);
-
-      other_way.MapIds(dst_id, next_src_id++);
+    if (!dst_to_src_.IsMapped(dst_id) && dst_insn_defined(dst_id)) {
+      uint32_t fresh_src_id = src_to_dst_.MakeFreshId();
+      MapIds(fresh_src_id, dst_id);
    }
  }
 }

-void SrcDstIdMap::MapUnmatchedIds() {
-  src_to_dst_.MapUnmatchedIds(dst_to_src_);
-}
-
 void IdInstructions::MapIdToInstruction(uint32_t id,
                                        const opt::Instruction* inst) {
  assert(id != 0);
@ -2735,7 +2736,9 @@ opt::Instruction Differ::ToMappedSrcIds(const opt::Instruction& dst_inst) {
 }

 spv_result_t Differ::Output() {
-  id_map_.MapUnmatchedIds();
+  id_map_.MapUnmatchedIds(
+      [this](uint32_t src_id) { return src_id_to_.IsDefined(src_id); },
+      [this](uint32_t dst_id) { return dst_id_to_.IsDefined(dst_id); });
  src_id_to_.inst_map_.resize(id_map_.SrcToDstMap().IdBound(), nullptr);
  dst_id_to_.inst_map_.resize(id_map_.DstToSrcMap().IdBound(), nullptr);

--- a/test/diff/diff_files/OpExtInst_in_src_only_autogen.cpp
+++ b/test/diff/diff_files/OpExtInst_in_src_only_autogen.cpp
@ -95,8 +95,7 @@ TEST(DiffTest, OpextinstInSrcOnly) {
  constexpr char kDiff[] = R"( ; SPIR-V
 ; Version: 1.6
 ; Generator: Khronos SPIR-V Tools Assembler; 0
-; Bound: 15
-+; Bound: 16
+ ; Bound: 15
 ; Schema: 0
 OpCapability Shader
 -%1 = OpExtInstImport "GLSL.std.450"
@ -199,8 +198,7 @@ TEST(DiffTest, OpextinstInSrcOnlyNoDebug) {
  constexpr char kDiff[] = R"( ; SPIR-V
 ; Version: 1.6
 ; Generator: Khronos SPIR-V Tools Assembler; 0
-; Bound: 15
-+; Bound: 16
+ ; Bound: 15
 ; Schema: 0
 OpCapability Shader
 -%1 = OpExtInstImport "GLSL.std.450"
--- a/test/diff/diff_files/basic_autogen.cpp
+++ b/test/diff/diff_files/basic_autogen.cpp
@ -129,7 +129,7 @@ TEST(DiffTest, Basic) {
 ; Version: 1.6
 ; Generator: Khronos SPIR-V Tools Assembler; 0
 -; Bound: 27
-+; Bound: 36
+; Bound: 30
 ; Schema: 0
 OpCapability Shader
 +%27 = OpExtInstImport "GLSL.std.450"
@ -272,7 +272,7 @@ OpFunctionEnd
 ; Version: 1.6
 ; Generator: Khronos SPIR-V Tools Assembler; 0
 -; Bound: 27
-+; Bound: 36
+; Bound: 30
 ; Schema: 0
 OpCapability Shader
 +%27 = OpExtInstImport "GLSL.std.450"
@ -324,7 +324,7 @@ TEST(DiffTest, BasicDumpIds) {
 ; Version: 1.6
 ; Generator: Khronos SPIR-V Tools Assembler; 0
 -; Bound: 27
-+; Bound: 36
+; Bound: 30
 ; Schema: 0
 OpCapability Shader
 +%27 = OpExtInstImport "GLSL.std.450"
@ -384,8 +384,8 @@ TEST(DiffTest, BasicDumpIds) {
   6 ->   14 [TypeInt]
  13 ->   19 [TypePointer]
  14 ->   27 [Variable]
-  15 ->   34 [Constant]
-  16 ->   35 [TypeArray]
+  15 ->   28 [Constant]
+  16 ->   29 [TypeArray]
  17 ->   11 [TypeStruct]
  18 ->   12 [TypePointer]
  19 ->   13 [Variable]
--- a/test/diff/diff_files/constant_array_size_autogen.cpp
+++ b/test/diff/diff_files/constant_array_size_autogen.cpp
@ -125,7 +125,7 @@ TEST(DiffTest, ConstantArraySize) {
 ; Version: 1.6
 ; Generator: Khronos SPIR-V Tools Assembler; 0
 -; Bound: 27
-+; Bound: 34
+; Bound: 28
 ; Schema: 0
 OpCapability Shader
 OpMemoryModel Logical GLSL450
@ -259,7 +259,7 @@ OpFunctionEnd
 ; Version: 1.6
 ; Generator: Khronos SPIR-V Tools Assembler; 0
 -; Bound: 27
-+; Bound: 34
+; Bound: 28
 ; Schema: 0
 OpCapability Shader
 OpMemoryModel Logical GLSL450
--- a/test/diff/diff_files/different_function_parameter_count_autogen.cpp
+++ b/test/diff/diff_files/different_function_parameter_count_autogen.cpp
@ -128,7 +128,7 @@ TEST(DiffTest, DifferentFunctionParameterCount) {
 ; Version: 1.6
 ; Generator: Khronos SPIR-V Tools Assembler; 0
 -; Bound: 25
-+; Bound: 33
+; Bound: 31
 ; Schema: 0
 OpCapability Shader
 %1 = OpExtInstImport "GLSL.std.450"
@ -143,7 +143,7 @@ TEST(DiffTest, DifferentFunctionParameterCount) {
 +OpName %26 "v2"
 OpName %20 "o"
 OpName %23 "param"
-+OpName %31 "param"
+OpName %29 "param"
 OpDecorate %20 RelaxedPrecision
 OpDecorate %20 Location 0
 %2 = OpTypeVoid
@ -162,13 +162,13 @@ TEST(DiffTest, DifferentFunctionParameterCount) {
 %4 = OpFunction %2 None %3
 %5 = OpLabel
 %23 = OpVariable %8 Function
-+%31 = OpVariable %8 Function
+%29 = OpVariable %8 Function
 OpStore %23 %22
 -%24 = OpFunctionCall %7 %11 %23
-+OpStore %31 %15
-+%32 = OpFunctionCall %7 %11 %23 %31
+OpStore %29 %15
+%30 = OpFunctionCall %7 %11 %23 %29
 -OpStore %20 %24
-+OpStore %20 %32
+OpStore %20 %30
 OpReturn
 OpFunctionEnd
 -%11 = OpFunction %7 None %9
@ -280,7 +280,7 @@ TEST(DiffTest, DifferentFunctionParameterCountNoDebug) {
 ; Version: 1.6
 ; Generator: Khronos SPIR-V Tools Assembler; 0
 -; Bound: 25
-+; Bound: 33
+; Bound: 31
 ; Schema: 0
 OpCapability Shader
 %1 = OpExtInstImport "GLSL.std.450"
@ -306,13 +306,13 @@ TEST(DiffTest, DifferentFunctionParameterCountNoDebug) {
 %4 = OpFunction %2 None %3
 %5 = OpLabel
 %23 = OpVariable %8 Function
-+%31 = OpVariable %8 Function
+%29 = OpVariable %8 Function
 OpStore %23 %22
 -%24 = OpFunctionCall %7 %11 %23
-+OpStore %31 %15
-+%32 = OpFunctionCall %7 %11 %23 %31
+OpStore %29 %15
+%30 = OpFunctionCall %7 %11 %23 %29
 -OpStore %20 %24
-+OpStore %20 %32
+OpStore %20 %30
 OpReturn
 OpFunctionEnd
 -%11 = OpFunction %7 None %9
--- a/test/diff/diff_files/extra_if_block_autogen.cpp
+++ b/test/diff/diff_files/extra_if_block_autogen.cpp
@ -303,7 +303,7 @@ TEST(DiffTest, ExtraIfBlock) {
 ; Version: 1.6
 ; Generator: Khronos SPIR-V Tools Assembler; 0
 -; Bound: 69
-+; Bound: 81
+; Bound: 77
 ; Schema: 0
 OpCapability Shader
 %1 = OpExtInstImport "GLSL.std.450"
@ -352,10 +352,10 @@ TEST(DiffTest, ExtraIfBlock) {
 OpDecorate %54 RelaxedPrecision
 OpDecorate %55 RelaxedPrecision
 OpDecorate %56 RelaxedPrecision
-+OpDecorate %72 RelaxedPrecision
+OpDecorate %70 RelaxedPrecision
 OpDecorate %57 RelaxedPrecision
-+OpDecorate %77 RelaxedPrecision
-+OpDecorate %78 RelaxedPrecision
+OpDecorate %75 RelaxedPrecision
+OpDecorate %76 RelaxedPrecision
 OpDecorate %58 RelaxedPrecision
 OpDecorate %63 RelaxedPrecision
 OpDecorate %63 Location 0
@ -383,7 +383,7 @@ TEST(DiffTest, ExtraIfBlock) {
 %32 = OpConstant %19 1
 %49 = OpConstant %6 10
 %52 = OpConstant %6 0.5
-+%76 = OpConstant %6 0.100000001
+%74 = OpConstant %6 0.100000001
 %53 = OpConstant %6 0.699999988
 %61 = OpTypeVector %6 4
 %62 = OpTypePointer Output %61
@ -439,20 +439,20 @@ TEST(DiffTest, ExtraIfBlock) {
 %55 = OpLoad %6 %45
 %56 = OpFMul %6 %55 %54
 OpStore %45 %56
-+%71 = OpAccessChain %21 %18 %32
-+%72 = OpLoad %15 %71
-+%73 = OpINotEqual %25 %72 %24
-+OpSelectionMerge %75 None
-+OpBranchConditional %73 %74 %75
-+%74 = OpLabel
+%69 = OpAccessChain %21 %18 %32
+%70 = OpLoad %15 %69
+%71 = OpINotEqual %25 %70 %24
+OpSelectionMerge %73 None
+OpBranchConditional %71 %72 %73
+%72 = OpLabel
 %57 = OpLoad %6 %45
-+%77 = OpFSub %6 %57 %76
-+OpStore %45 %77
-+OpBranch %75
-+%75 = OpLabel
-+%78 = OpLoad %6 %45
+%75 = OpFSub %6 %57 %74
+OpStore %45 %75
+OpBranch %73
+%73 = OpLabel
+%76 = OpLoad %6 %45
 -%58 = OpExtInst %6 %1 Exp %57
-+%58 = OpExtInst %6 %1 Exp %78
+%58 = OpExtInst %6 %1 Exp %76
 OpReturnValue %58
 OpFunctionEnd
 )";
@ -716,7 +716,7 @@ TEST(DiffTest, ExtraIfBlockNoDebug) {
 ; Version: 1.6
 ; Generator: Khronos SPIR-V Tools Assembler; 0
 -; Bound: 69
-+; Bound: 81
+; Bound: 77
 ; Schema: 0
 OpCapability Shader
 %1 = OpExtInstImport "GLSL.std.450"
@ -754,10 +754,10 @@ TEST(DiffTest, ExtraIfBlockNoDebug) {
 OpDecorate %54 RelaxedPrecision
 OpDecorate %55 RelaxedPrecision
 OpDecorate %56 RelaxedPrecision
-+OpDecorate %72 RelaxedPrecision
+OpDecorate %70 RelaxedPrecision
 OpDecorate %57 RelaxedPrecision
-+OpDecorate %77 RelaxedPrecision
-+OpDecorate %78 RelaxedPrecision
+OpDecorate %75 RelaxedPrecision
+OpDecorate %76 RelaxedPrecision
 OpDecorate %58 RelaxedPrecision
 OpDecorate %63 RelaxedPrecision
 OpDecorate %63 Location 0
@ -785,7 +785,7 @@ TEST(DiffTest, ExtraIfBlockNoDebug) {
 %32 = OpConstant %19 1
 %49 = OpConstant %6 10
 %52 = OpConstant %6 0.5
-+%76 = OpConstant %6 0.100000001
+%74 = OpConstant %6 0.100000001
 %53 = OpConstant %6 0.699999988
 %61 = OpTypeVector %6 4
 %62 = OpTypePointer Output %61
@ -841,20 +841,20 @@ TEST(DiffTest, ExtraIfBlockNoDebug) {
 %55 = OpLoad %6 %45
 %56 = OpFMul %6 %55 %54
 OpStore %45 %56
-+%71 = OpAccessChain %21 %18 %32
-+%72 = OpLoad %15 %71
-+%73 = OpINotEqual %25 %72 %24
-+OpSelectionMerge %75 None
-+OpBranchConditional %73 %74 %75
-+%74 = OpLabel
+%69 = OpAccessChain %21 %18 %32
+%70 = OpLoad %15 %69
+%71 = OpINotEqual %25 %70 %24
+OpSelectionMerge %73 None
+OpBranchConditional %71 %72 %73
+%72 = OpLabel
 %57 = OpLoad %6 %45
-+%77 = OpFSub %6 %57 %76
-+OpStore %45 %77
-+OpBranch %75
-+%75 = OpLabel
-+%78 = OpLoad %6 %45
+%75 = OpFSub %6 %57 %74
+OpStore %45 %75
+OpBranch %73
+%73 = OpLabel
+%76 = OpLoad %6 %45
 -%58 = OpExtInst %6 %1 Exp %57
-+%58 = OpExtInst %6 %1 Exp %78
+%58 = OpExtInst %6 %1 Exp %76
 OpReturnValue %58
 OpFunctionEnd
 )";
--- a/test/diff/diff_files/int_vs_uint_constants_autogen.cpp
+++ b/test/diff/diff_files/int_vs_uint_constants_autogen.cpp
@ -371,10 +371,10 @@ TEST(DiffTest, IntVsUintConstantsDumpIds) {
   3 ->   16 [TypePointer]
   4 ->   17 [Variable]
   5 ->    8 [TypeInt]
-   8 ->   23 [TypeVector]
+   8 ->   21 [TypeVector]
  13 ->   19 [TypePointer]
-  15 ->   29 [Constant]
-  16 ->   30 [TypeArray]
+  15 ->   22 [Constant]
+  16 ->   23 [TypeArray]
  17 ->   11 [TypeStruct]
  18 ->   12 [TypePointer]
  19 ->   13 [Variable]
--- a/test/diff/diff_files/reordered_if_blocks_autogen.cpp
+++ b/test/diff/diff_files/reordered_if_blocks_autogen.cpp
@ -203,8 +203,7 @@ TEST(DiffTest, ReorderedIfBlocks) {
  constexpr char kDiff[] = R"( ; SPIR-V
 ; Version: 1.6
 ; Generator: Khronos SPIR-V Tools Assembler; 0
-; Bound: 46
-+; Bound: 47
+ ; Bound: 46
 ; Schema: 0
 OpCapability Shader
 %1 = OpExtInstImport "GLSL.std.450"
@ -471,8 +470,7 @@ TEST(DiffTest, ReorderedIfBlocksNoDebug) {
  constexpr char kDiff[] = R"( ; SPIR-V
 ; Version: 1.6
 ; Generator: Khronos SPIR-V Tools Assembler; 0
-; Bound: 46
-+; Bound: 47
+ ; Bound: 46
 ; Schema: 0
 OpCapability Shader
 %1 = OpExtInstImport "GLSL.std.450"
--- a/test/diff/diff_files/reordered_switch_blocks_autogen.cpp
+++ b/test/diff/diff_files/reordered_switch_blocks_autogen.cpp
@ -212,8 +212,7 @@ TEST(DiffTest, ReorderedSwitchBlocks) {
  constexpr char kDiff[] = R"( ; SPIR-V
 ; Version: 1.6
 ; Generator: Khronos SPIR-V Tools Assembler; 0
-; Bound: 58
-+; Bound: 62
+ ; Bound: 58
 ; Schema: 0
 OpCapability Shader
 %1 = OpExtInstImport "GLSL.std.450"
@ -485,8 +484,7 @@ TEST(DiffTest, ReorderedSwitchBlocksNoDebug) {
  constexpr char kDiff[] = R"( ; SPIR-V
 ; Version: 1.6
 ; Generator: Khronos SPIR-V Tools Assembler; 0
-; Bound: 58
-+; Bound: 62
+ ; Bound: 58
 ; Schema: 0
 OpCapability Shader
 %1 = OpExtInstImport "GLSL.std.450"
--- a/test/diff/diff_files/spec_constant_array_size_autogen.cpp
+++ b/test/diff/diff_files/spec_constant_array_size_autogen.cpp
@ -125,7 +125,7 @@ TEST(DiffTest, SpecConstantArraySize) {
 ; Version: 1.6
 ; Generator: Khronos SPIR-V Tools Assembler; 0
 -; Bound: 27
-+; Bound: 36
+; Bound: 29
 ; Schema: 0
 OpCapability Shader
 OpMemoryModel Logical GLSL450
@ -140,7 +140,7 @@ TEST(DiffTest, SpecConstantArraySize) {
 OpName %19 ""
 OpName %22 "main"
 OpDecorate %4 Location 0
-+OpDecorate %34 SpecId 4
+OpDecorate %27 SpecId 4
 OpMemberDecorate %17 1 RelaxedPrecision
 OpMemberDecorate %17 0 BuiltIn Position
 OpMemberDecorate %17 1 BuiltIn PointSize
@ -153,10 +153,10 @@ TEST(DiffTest, SpecConstantArraySize) {
 %8 = OpTypeVector %5 4
 -%15 = OpConstant %5 8
 -%16 = OpTypeArray %1 %15
-+%34 = OpSpecConstant %5 8
-+%35 = OpTypeArray %1 %34
+%27 = OpSpecConstant %5 8
+%28 = OpTypeArray %1 %27
 -%17 = OpTypeStruct %2 %1 %16 %16
-+%17 = OpTypeStruct %2 %1 %35 %35
+%17 = OpTypeStruct %2 %1 %28 %28
 %20 = OpTypeVoid
 %25 = OpConstant %5 0
 %3 = OpTypePointer Input %2
@ -261,14 +261,14 @@ OpFunctionEnd
 ; Version: 1.6
 ; Generator: Khronos SPIR-V Tools Assembler; 0
 -; Bound: 27
-+; Bound: 36
+; Bound: 29
 ; Schema: 0
 OpCapability Shader
 OpMemoryModel Logical GLSL450
 OpEntryPoint Vertex %22 "main" %4 %19
 OpSource GLSL 450
 OpDecorate %4 Location 0
-+OpDecorate %34 SpecId 4
+OpDecorate %27 SpecId 4
 OpMemberDecorate %17 1 RelaxedPrecision
 OpMemberDecorate %17 0 BuiltIn Position
 OpMemberDecorate %17 1 BuiltIn PointSize
@ -281,10 +281,10 @@ OpFunctionEnd
 %8 = OpTypeVector %5 4
 -%15 = OpConstant %5 8
 -%16 = OpTypeArray %1 %15
-+%34 = OpSpecConstant %5 8
-+%35 = OpTypeArray %1 %34
+%27 = OpSpecConstant %5 8
+%28 = OpTypeArray %1 %27
 -%17 = OpTypeStruct %2 %1 %16 %16
-+%17 = OpTypeStruct %2 %1 %35 %35
+%17 = OpTypeStruct %2 %1 %28 %28
 %20 = OpTypeVoid
 %25 = OpConstant %5 0
 %3 = OpTypePointer Input %2