Skip to content

Commit

Permalink
Fix pairing of function parameters.
Browse files Browse the repository at this point in the history 
- Consider prior type pairings when attempting to pair function
parameters by type.
- Pair all parameters that have matching types, not just the first.
- Update diff tests.

Fixes #5218.
  • Loading branch information
@jimblandy
jimblandy committed May 21, 2023
1 parent e7c6084 commit 09888a1
Show file tree
Hide file tree
Showing 4 changed files with 118 additions and 81 deletions.
62 changes: 59 additions & 3 deletions source/diff/diff.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,30 @@ class Differ {
std::function<void(const IdGroup& src_group, const IdGroup& dst_group)>
match_group);

// Bucket `src_ids` and `dst_ids` by key ids returned by `get_group`, and
// then pair up buckets whose ids map to each other.
//
// For example, say the input groups are as follows, where `X -> Y` means
// that `X` is a member of the `IdGroup` and `get_group` maps `X` to `Y`
//
// - src_ids: { 1 -> 100, 2 -> 300, 3 -> 100, 4 -> 200 }
// - dst_ids: { 5 -> 10, 6 -> 20, 7 -> 10, 8 -> 300 }
//
// and suppose that the following src/dst ids are paired: 100/10 and 200/20.
//
// Then `match_group` is called twice:
// - Once with ([1,3], [5, 7]), corresponding to 100/10
// - Once with ([4,6]), corresponding to 200/20
//
// The source id 300 isn't paired with anything, so the fact that there's
// a destination id 300 is irrelevant, and thus 2 and 8 are never passed
// to `match_group`.
void GroupIdsAndMatchByMappedId(
const IdGroup& src_ids, const IdGroup& dst_ids,
uint32_t (Differ::*get_group)(const IdInstructions&, uint32_t),
std::function<void(const IdGroup& src_group, const IdGroup& dst_group)>
match_group);

// Helper functions that determine if two instructions match
bool DoIdsMatch(uint32_t src_id, uint32_t dst_id);
bool DoesOperandMatch(const opt::Operand& src_operand,
Expand Down Expand Up @@ -889,6 +913,37 @@ void Differ::GroupIdsAndMatch(
}
}

void Differ::GroupIdsAndMatchByMappedId(
const IdGroup& src_ids, const IdGroup& dst_ids,
uint32_t (Differ::*get_group)(const IdInstructions&, uint32_t),
std::function<void(const IdGroup& src_group, const IdGroup& dst_group)>
match_group) {
// Group the ids based on a key (get_group)
std::map<uint32_t, IdGroup> src_groups;
std::map<uint32_t, IdGroup> dst_groups;

GroupIds<uint32_t>(src_ids, true, &src_groups, get_group);
GroupIds<uint32_t>(dst_ids, false, &dst_groups, get_group);

// Iterate over pairs of groups whose keys map to each other.
for (const auto& iter : src_groups) {
const uint32_t& src_key = iter.first;
const IdGroup& src_group = iter.second;

if (src_key == 0) {
continue;
}

if (id_map_.IsSrcMapped(src_key)) {
const uint32_t& dst_key = id_map_.MappedDstId(src_key);
const IdGroup& dst_group = dst_groups[dst_key];

// Let the caller match the groups as appropriate.
match_group(src_group, dst_group);
}
}
}

bool Differ::DoIdsMatch(uint32_t src_id, uint32_t dst_id) {
assert(dst_id != 0);
return id_map_.MappedDstId(src_id) == dst_id;
Expand Down Expand Up @@ -1574,6 +1629,7 @@ void Differ::BestEffortMatchFunctions(const IdGroup& src_func_ids,

id_map_.MapIds(match_result.src_id, match_result.dst_id);

MatchFunctionParamIds(src_funcs_[match_result.src_id], dst_funcs_[match_result.dst_id]);
MatchIdsInFunctionBodies(src_func_insts.at(match_result.src_id),
dst_func_insts.at(match_result.dst_id),
match_result.src_match, match_result.dst_match, 0);
Expand Down Expand Up @@ -1609,8 +1665,8 @@ void Differ::MatchFunctionParamIds(const opt::Function* src_func,

// Then match the parameters by their type. If there are multiple of them,
// match them by their order.
GroupIdsAndMatch<uint32_t>(
src_params, dst_params, 0, &Differ::GroupIdsHelperGetTypeId,
GroupIdsAndMatchByMappedId(
src_params, dst_params, &Differ::GroupIdsHelperGetTypeId,
[this](const IdGroup& src_group_by_type_id,
const IdGroup& dst_group_by_type_id) {

Expand All @@ -1619,7 +1675,7 @@ void Differ::MatchFunctionParamIds(const opt::Function* src_func,

for (size_t param_index = 0; param_index < shared_param_count;
++param_index) {
id_map_.MapIds(src_group_by_type_id[0], dst_group_by_type_id[0]);
id_map_.MapIds(src_group_by_type_id[param_index], dst_group_by_type_id[param_index]);
}
});
}
Expand Down
44 changes: 17 additions & 27 deletions test/diff/diff_files/different_decorations_fragment_autogen.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -977,7 +977,7 @@ OpFunctionEnd
; Version: 1.6
; Generator: Khronos SPIR-V Tools Assembler; 0
-; Bound: 82
+; Bound: 92
+; Bound: 89
; Schema: 0
OpCapability Shader
OpMemoryModel Logical GLSL450
Expand Down Expand Up @@ -1030,8 +1030,7 @@ OpFunctionEnd
+OpDecorate %83 DescriptorSet 0
+OpDecorate %83 Binding 0
OpDecorate %32 RelaxedPrecision
-OpDecorate %33 RelaxedPrecision
+OpDecorate %84 RelaxedPrecision
OpDecorate %33 RelaxedPrecision
OpDecorate %36 RelaxedPrecision
OpDecorate %37 RelaxedPrecision
OpDecorate %38 RelaxedPrecision
Expand All @@ -1040,10 +1039,8 @@ OpFunctionEnd
OpDecorate %42 RelaxedPrecision
OpDecorate %43 RelaxedPrecision
OpDecorate %48 RelaxedPrecision
-OpDecorate %49 RelaxedPrecision
-OpDecorate %50 RelaxedPrecision
+OpDecorate %85 RelaxedPrecision
+OpDecorate %86 RelaxedPrecision
OpDecorate %49 RelaxedPrecision
OpDecorate %50 RelaxedPrecision
OpDecorate %52 RelaxedPrecision
OpDecorate %53 RelaxedPrecision
OpDecorate %54 RelaxedPrecision
Expand Down Expand Up @@ -1082,13 +1079,13 @@ OpFunctionEnd
%61 = OpTypeVoid
%69 = OpConstant %16 0
%78 = OpConstant %16 1
+%88 = OpTypePointer Private %2
+%85 = OpTypePointer Private %2
%3 = OpTypePointer Input %2
%7 = OpTypePointer UniformConstant %6
%10 = OpTypePointer UniformConstant %9
%13 = OpTypePointer Uniform %12
%19 = OpTypePointer Uniform %18
+%89 = OpTypePointer Private %2
+%86 = OpTypePointer Private %2
%21 = OpTypePointer Output %2
%28 = OpTypePointer Uniform %27
%30 = OpTypePointer Function %2
Expand All @@ -1106,19 +1103,16 @@ OpFunctionEnd
%22 = OpVariable %21 Output
-%29 = OpVariable %28 Uniform
+%83 = OpVariable %28 Uniform
+%90 = OpConstant %23 0
+%91 = OpConstant %1 0.5
+%87 = OpConstant %23 0
+%88 = OpConstant %1 0.5
%32 = OpFunction %2 None %31
-%33 = OpFunctionParameter %30
+%84 = OpFunctionParameter %30
%33 = OpFunctionParameter %30
%34 = OpLabel
%36 = OpLoad %6 %8
-%37 = OpLoad %2 %33
+%37 = OpLoad %2 %84
%37 = OpLoad %2 %33
%38 = OpVectorShuffle %35 %37 %37 0 1
%39 = OpImageSampleImplicitLod %2 %36 %38
-%41 = OpLoad %2 %33
+%41 = OpLoad %2 %84
%41 = OpLoad %2 %33
%42 = OpVectorShuffle %35 %41 %41 2 3
%43 = OpConvertFToS %40 %42
%44 = OpLoad %9 %11
Expand All @@ -1127,16 +1121,12 @@ OpFunctionEnd
OpReturnValue %46
OpFunctionEnd
%48 = OpFunction %2 None %47
-%49 = OpFunctionParameter %30
-%50 = OpFunctionParameter %30
+%85 = OpFunctionParameter %30
+%86 = OpFunctionParameter %30
%49 = OpFunctionParameter %30
%50 = OpFunctionParameter %30
%51 = OpLabel
-%52 = OpLoad %2 %49
+%52 = OpLoad %2 %85
%52 = OpLoad %2 %49
%53 = OpVectorShuffle %35 %52 %52 0 1
-%54 = OpLoad %2 %50
+%54 = OpLoad %2 %86
%54 = OpLoad %2 %50
%55 = OpVectorShuffle %35 %54 %54 2 3
%56 = OpCompositeExtract %1 %53 0
%57 = OpCompositeExtract %1 %53 1
Expand All @@ -1154,9 +1144,9 @@ OpFunctionEnd
OpStore %65 %66
%67 = OpFunctionCall %2 %32 %65
-%71 = OpAccessChain %70 %14 %69
+%87 = OpAccessChain %70 %82 %69
+%84 = OpAccessChain %70 %82 %69
-%72 = OpLoad %2 %71
+%72 = OpLoad %2 %87
+%72 = OpLoad %2 %84
OpStore %68 %72
-%74 = OpAccessChain %70 %20 %69 %69
+%74 = OpAccessChain %70 %14 %69 %69
Expand Down
71 changes: 32 additions & 39 deletions test/diff/diff_files/different_decorations_vertex_autogen.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -777,7 +777,7 @@ OpFunctionEnd
; Version: 1.6
; Generator: Khronos SPIR-V Tools Assembler; 0
-; Bound: 58
+; Bound: 79
+; Bound: 77
; Schema: 0
OpCapability Shader
OpMemoryModel Logical GLSL450
Expand Down Expand Up @@ -817,12 +817,10 @@ OpFunctionEnd
-OpMemberDecorate %23 3 BuiltIn CullDistance
OpDecorate %23 Block
OpDecorate %28 RelaxedPrecision
-OpDecorate %29 RelaxedPrecision
+OpDecorate %59 RelaxedPrecision
OpDecorate %29 RelaxedPrecision
OpDecorate %31 RelaxedPrecision
OpDecorate %32 RelaxedPrecision
-OpDecorate %33 RelaxedPrecision
+OpDecorate %60 RelaxedPrecision
OpDecorate %33 RelaxedPrecision
OpDecorate %35 RelaxedPrecision
OpDecorate %36 RelaxedPrecision
OpDecorate %37 RelaxedPrecision
Expand All @@ -845,9 +843,9 @@ OpFunctionEnd
+%23 = OpTypeStruct %2
%38 = OpTypeVoid
%45 = OpConstant %12 0
+%65 = OpTypePointer Private %2
+%63 = OpTypePointer Private %2
%3 = OpTypePointer Input %2
+%66 = OpTypePointer Private %2
+%64 = OpTypePointer Private %2
%7 = OpTypePointer Output %2
%10 = OpTypePointer Uniform %9
%18 = OpTypePointer Uniform %17
Expand All @@ -865,26 +863,21 @@ OpFunctionEnd
-%19 = OpVariable %18 Uniform
+%19 = OpVariable %10 Uniform
%20 = OpVariable %7 Output
+%58 = OpVariable %66 Private
+%58 = OpVariable %64 Private
%25 = OpVariable %24 Output
+%67 = OpConstant %13 0
+%68 = OpConstant %1 0.5
+%65 = OpConstant %13 0
+%66 = OpConstant %1 0.5
%28 = OpFunction %2 None %27
-%29 = OpFunctionParameter %26
+%59 = OpFunctionParameter %26
%29 = OpFunctionParameter %26
%30 = OpLabel
-%31 = OpLoad %2 %29
+%31 = OpLoad %2 %59
%31 = OpLoad %2 %29
OpReturnValue %31
OpFunctionEnd
%32 = OpFunction %2 None %27
-%33 = OpFunctionParameter %26
+%60 = OpFunctionParameter %26
%33 = OpFunctionParameter %26
%34 = OpLabel
-%35 = OpLoad %2 %33
+%35 = OpLoad %2 %60
-%36 = OpLoad %2 %33
+%36 = OpLoad %2 %60
%35 = OpLoad %2 %33
%36 = OpLoad %2 %33
%37 = OpFAdd %2 %35 %36
OpReturnValue %37
OpFunctionEnd
Expand All @@ -894,41 +887,41 @@ OpFunctionEnd
%50 = OpVariable %26 Function
%53 = OpVariable %26 Function
-%43 = OpLoad %2 %4
+%61 = OpLoad %2 %5
+%59 = OpLoad %2 %5
-OpStore %42 %43
+OpStore %42 %61
+OpStore %42 %59
%44 = OpFunctionCall %2 %28 %42
-%47 = OpAccessChain %46 %11 %45
+%62 = OpAccessChain %46 %19 %45
+%60 = OpAccessChain %46 %19 %45
-%48 = OpLoad %2 %47
+%48 = OpLoad %2 %62
+%48 = OpLoad %2 %60
%49 = OpFAdd %2 %44 %48
-OpStore %8 %49
+OpStore %20 %49
-%51 = OpLoad %2 %5
+%63 = OpLoad %2 %6
+%61 = OpLoad %2 %6
-OpStore %50 %51
+OpStore %50 %63
+OpStore %50 %61
%52 = OpFunctionCall %2 %32 %50
-%54 = OpLoad %2 %6
+%64 = OpLoad %2 %4
+%62 = OpLoad %2 %4
-OpStore %53 %54
+OpStore %53 %64
+OpStore %53 %62
%55 = OpFunctionCall %2 %28 %53
%56 = OpFAdd %2 %52 %55
%57 = OpAccessChain %7 %25 %45
OpStore %57 %56
+%69 = OpAccessChain %7 %25 %67
+%70 = OpLoad %2 %69
+%71 = OpCompositeExtract %1 %70 0
+%72 = OpCompositeExtract %1 %70 1
+%73 = OpCompositeExtract %1 %70 2
+%74 = OpCompositeExtract %1 %70 3
+%76 = OpFNegate %1 %71
+%77 = OpFAdd %1 %73 %74
+%78 = OpFMul %1 %77 %68
+%75 = OpCompositeConstruct %2 %72 %76 %78 %74
+OpStore %69 %75
+%67 = OpAccessChain %7 %25 %65
+%68 = OpLoad %2 %67
+%69 = OpCompositeExtract %1 %68 0
+%70 = OpCompositeExtract %1 %68 1
+%71 = OpCompositeExtract %1 %68 2
+%72 = OpCompositeExtract %1 %68 3
+%74 = OpFNegate %1 %69
+%75 = OpFAdd %1 %71 %72
+%76 = OpFMul %1 %75 %66
+%73 = OpCompositeConstruct %2 %70 %74 %76 %72
+OpStore %67 %73
OpReturn
OpFunctionEnd
)";
Expand Down
22 changes: 10 additions & 12 deletions test/diff/diff_files/different_function_parameter_count_autogen.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ TEST(DiffTest, DifferentFunctionParameterCountNoDebug) {
; Version: 1.6
; Generator: Khronos SPIR-V Tools Assembler; 0
-; Bound: 25
+; Bound: 34
+; Bound: 33
; Schema: 0
OpCapability Shader
%1 = OpExtInstImport "GLSL.std.450"
Expand All @@ -306,28 +306,26 @@ TEST(DiffTest, DifferentFunctionParameterCountNoDebug) {
%4 = OpFunction %2 None %3
%5 = OpLabel
%23 = OpVariable %8 Function
+%32 = OpVariable %8 Function
+%31 = OpVariable %8 Function
OpStore %23 %22
-%24 = OpFunctionCall %7 %11 %23
+OpStore %32 %15
+%33 = OpFunctionCall %7 %11 %23 %32
+OpStore %31 %15
+%32 = OpFunctionCall %7 %11 %23 %31
-OpStore %20 %24
+OpStore %20 %33
+OpStore %20 %32
OpReturn
OpFunctionEnd
-%11 = OpFunction %7 None %9
+%11 = OpFunction %7 None %25
-%10 = OpFunctionParameter %8
%10 = OpFunctionParameter %8
+%26 = OpFunctionParameter %8
+%27 = OpFunctionParameter %8
%12 = OpLabel
-%13 = OpLoad %7 %10
+%13 = OpLoad %7 %26
%13 = OpLoad %7 %10
-%16 = OpFAdd %7 %13 %15
+%28 = OpLoad %7 %27
+%29 = OpFAdd %7 %13 %28
+%27 = OpLoad %7 %26
+%28 = OpFAdd %7 %13 %27
-OpReturnValue %16
+OpReturnValue %29
+OpReturnValue %28
OpFunctionEnd
)";
Options options;
Expand Down

0 comments on commit 09888a1

Please sign in to comment.