Fix pairing of functions by type.

Change `Differ::MatchFunctions` to use `GroupIdsAndMatchByMappedId`, instead of calling `GroupIdsAndMatch` with `Differ::GroupIdsHelperGetTypeId`, which is never correct: doing so compares src and dst type ids directly, rather than consulting the established mapping. Fixes #5226.
KhronosGroup · Feb 28, 2025 · 659cc54 · 659cc54
1 parent b1140ad
commit 659cc54
Show file tree

Hide file tree

Showing 5 changed files with 230 additions and 3 deletions.
diff --git a/source/diff/diff.cpp b/source/diff/diff.cpp
@@ -2487,7 +2487,7 @@ void Differ::MatchFunctions() {
 
         // If there are multiple functions with the same name, group them by
         // type, and match only if the types match (and are unique).
-        GroupIdsAndMatch<uint32_t>(src_group, dst_group, 0,
+        GroupIdsAndMatchByMappedId(src_group, dst_group,
                                    &Differ::GroupIdsHelperGetTypeId,
                                    [this](const IdGroup& src_group_by_type_id,
                                           const IdGroup& dst_group_by_type_id) {
@@ -2531,8 +2531,8 @@ void Differ::MatchFunctions() {
   }
 
   // Best effort match functions with matching type.
-  GroupIdsAndMatch<uint32_t>(
-      src_func_ids, dst_func_ids, 0, &Differ::GroupIdsHelperGetTypeId,
+  GroupIdsAndMatchByMappedId(
+      src_func_ids, dst_func_ids, &Differ::GroupIdsHelperGetTypeId,
       [this](const IdGroup& src_group_by_type_id,
              const IdGroup& dst_group_by_type_id) {
         BestEffortMatchFunctions(src_group_by_type_id, dst_group_by_type_id,

diff --git a/test/diff/diff_files/diff_test_files_autogen.cmake b/test/diff/diff_files/diff_test_files_autogen.cmake
@@ -30,6 +30,7 @@ list(APPEND DIFF_TEST_FILES
 "diff_files/different_decorations_vertex_autogen.cpp"
 "diff_files/different_function_parameter_count_autogen.cpp"
 "diff_files/extra_if_block_autogen.cpp"
+"diff_files/function_group_by_mapped_id_autogen.cpp"
 "diff_files/index_signedness_autogen.cpp"
 "diff_files/int_vs_uint_constants_autogen.cpp"
 "diff_files/large_functions_large_diffs_autogen.cpp"

diff --git a/test/diff/diff_files/function_group_by_mapped_id_autogen.cpp b/test/diff/diff_files/function_group_by_mapped_id_autogen.cpp
@@ -0,0 +1,180 @@
+// GENERATED FILE - DO NOT EDIT.
+// Generated by generate_tests.py
+//
+// Copyright (c) 2022 Google LLC.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "../diff_test_utils.h"
+
+#include "gtest/gtest.h"
+
+namespace spvtools {
+namespace diff {
+namespace {
+
+// Don't forget to map between source and destination ids when grouping functions by return type.
+constexpr char kSrc[] = R"(                 OpCapability Shader
+                 OpCapability Linkage
+                 OpMemoryModel Logical GLSL450
+         %void = OpTypeVoid               
+            %1 = OpTypeInt 32 0
+            %2 = OpTypeInt 32 1
+
+      %fn_uint = OpTypeFunction %1
+       %fn_int = OpTypeFunction %2
+
+      %uint_42 = OpConstant %1 42
+     %int_1729 = OpConstant %2 1729
+
+            %f = OpFunction %1 None %fn_uint
+   %f_prologue = OpLabel
+                 OpReturnValue %uint_42
+                 OpFunctionEnd
+
+            %g = OpFunction %2 None %fn_int
+   %g_prologue = OpLabel
+                 OpReturnValue %int_1729
+                 OpFunctionEnd)";
+constexpr char kDst[] = R"(;; Don't forget to map between source and destination ids when grouping functions by return type.
+                 OpCapability Shader
+                 OpCapability Linkage
+                 OpMemoryModel Logical GLSL450
+         %void = OpTypeVoid               
+            %2 = OpTypeInt 32 0
+            %1 = OpTypeInt 32 1
+
+      %fn_uint = OpTypeFunction %2
+       %fn_int = OpTypeFunction %1
+
+      %uint_42 = OpConstant %2 42
+     %int_1729 = OpConstant %1 1729
+
+            %f = OpFunction %2 None %fn_uint
+   %f_prologue = OpLabel
+                 OpReturnValue %uint_42
+                 OpFunctionEnd
+
+            %g = OpFunction %1 None %fn_int
+   %g_prologue = OpLabel
+                 OpReturnValue %int_1729
+                 OpFunctionEnd
+)";
+
+TEST(DiffTest, FunctionGroupByMappedId) {
+  constexpr char kDiff[] = R"( ; SPIR-V
+ ; Version: 1.6
+ ; Generator: Khronos SPIR-V Tools Assembler; 0
+ ; Bound: 12
+ ; Schema: 0
+ OpCapability Shader
+ OpCapability Linkage
+ OpMemoryModel Logical GLSL450
+ %3 = OpTypeVoid
+ %1 = OpTypeInt 32 0
+ %2 = OpTypeInt 32 1
+ %4 = OpTypeFunction %1
+ %5 = OpTypeFunction %2
+ %6 = OpConstant %1 42
+ %7 = OpConstant %2 1729
+ %8 = OpFunction %1 None %4
+ %9 = OpLabel
+ OpReturnValue %6
+ OpFunctionEnd
+ %10 = OpFunction %2 None %5
+ %11 = OpLabel
+ OpReturnValue %7
+ OpFunctionEnd
+)";
+  Options options;
+  DoStringDiffTest(kSrc, kDst, kDiff, options);
+}
+
+TEST(DiffTest, FunctionGroupByMappedIdNoDebug) {
+  constexpr char kSrcNoDebug[] = R"(                 OpCapability Shader
+                 OpCapability Linkage
+                 OpMemoryModel Logical GLSL450
+         %void = OpTypeVoid               
+            %1 = OpTypeInt 32 0
+            %2 = OpTypeInt 32 1
+
+      %fn_uint = OpTypeFunction %1
+       %fn_int = OpTypeFunction %2
+
+      %uint_42 = OpConstant %1 42
+     %int_1729 = OpConstant %2 1729
+
+            %f = OpFunction %1 None %fn_uint
+   %f_prologue = OpLabel
+                 OpReturnValue %uint_42
+                 OpFunctionEnd
+
+            %g = OpFunction %2 None %fn_int
+   %g_prologue = OpLabel
+                 OpReturnValue %int_1729
+                 OpFunctionEnd
+)";
+  constexpr char kDstNoDebug[] = R"(                 OpCapability Shader
+                 OpCapability Linkage
+                 OpMemoryModel Logical GLSL450
+         %void = OpTypeVoid               
+            %2 = OpTypeInt 32 0
+            %1 = OpTypeInt 32 1
+
+      %fn_uint = OpTypeFunction %2
+       %fn_int = OpTypeFunction %1
+
+      %uint_42 = OpConstant %2 42
+     %int_1729 = OpConstant %1 1729
+
+            %f = OpFunction %2 None %fn_uint
+   %f_prologue = OpLabel
+                 OpReturnValue %uint_42
+                 OpFunctionEnd
+
+            %g = OpFunction %1 None %fn_int
+   %g_prologue = OpLabel
+                 OpReturnValue %int_1729
+                 OpFunctionEnd
+)";
+  constexpr char kDiff[] = R"( ; SPIR-V
+ ; Version: 1.6
+ ; Generator: Khronos SPIR-V Tools Assembler; 0
+ ; Bound: 12
+ ; Schema: 0
+ OpCapability Shader
+ OpCapability Linkage
+ OpMemoryModel Logical GLSL450
+ %3 = OpTypeVoid
+ %1 = OpTypeInt 32 0
+ %2 = OpTypeInt 32 1
+ %4 = OpTypeFunction %1
+ %5 = OpTypeFunction %2
+ %6 = OpConstant %1 42
+ %7 = OpConstant %2 1729
+ %8 = OpFunction %1 None %4
+ %9 = OpLabel
+ OpReturnValue %6
+ OpFunctionEnd
+ %10 = OpFunction %2 None %5
+ %11 = OpLabel
+ OpReturnValue %7
+ OpFunctionEnd
+)";
+  Options options;
+  DoStringDiffTest(kSrcNoDebug, kDstNoDebug, kDiff, options);
+}
+
+}  // namespace
+}  // namespace diff
+}  // namespace spvtools
diff --git a/test/diff/diff_files/function_group_by_mapped_id_dst.spvasm b/test/diff/diff_files/function_group_by_mapped_id_dst.spvasm
@@ -0,0 +1,23 @@
+;; Don't forget to map between source and destination ids when grouping functions by return type.
+                 OpCapability Shader
+                 OpCapability Linkage
+                 OpMemoryModel Logical GLSL450
+         %void = OpTypeVoid               
+            %2 = OpTypeInt 32 0
+            %1 = OpTypeInt 32 1
+
+      %fn_uint = OpTypeFunction %2
+       %fn_int = OpTypeFunction %1
+
+      %uint_42 = OpConstant %2 42
+     %int_1729 = OpConstant %1 1729
+
+            %f = OpFunction %2 None %fn_uint
+   %f_prologue = OpLabel
+                 OpReturnValue %uint_42
+                 OpFunctionEnd
+
+            %g = OpFunction %1 None %fn_int
+   %g_prologue = OpLabel
+                 OpReturnValue %int_1729
+                 OpFunctionEnd
diff --git a/test/diff/diff_files/function_group_by_mapped_id_src.spvasm b/test/diff/diff_files/function_group_by_mapped_id_src.spvasm
@@ -0,0 +1,23 @@
+;; Don't forget to map between source and destination ids when grouping functions by return type.
+                 OpCapability Shader
+                 OpCapability Linkage
+                 OpMemoryModel Logical GLSL450
+         %void = OpTypeVoid               
+            %1 = OpTypeInt 32 0
+            %2 = OpTypeInt 32 1
+
+      %fn_uint = OpTypeFunction %1
+       %fn_int = OpTypeFunction %2
+
+      %uint_42 = OpConstant %1 42
+     %int_1729 = OpConstant %2 1729
+
+            %f = OpFunction %1 None %fn_uint
+   %f_prologue = OpLabel
+                 OpReturnValue %uint_42
+                 OpFunctionEnd
+
+            %g = OpFunction %2 None %fn_int
+   %g_prologue = OpLabel
+                 OpReturnValue %int_1729
+                 OpFunctionEnd