From 09888a153ee34d816fd286ed10c836e5654307d5 Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Sat, 20 May 2023 15:52:29 -0700 Subject: [PATCH] Fix pairing of function parameters. - 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. --- source/diff/diff.cpp | 62 +++++++++++++++- ...different_decorations_fragment_autogen.cpp | 44 +++++------- .../different_decorations_vertex_autogen.cpp | 71 +++++++++---------- ...erent_function_parameter_count_autogen.cpp | 22 +++--- 4 files changed, 118 insertions(+), 81 deletions(-) diff --git a/source/diff/diff.cpp b/source/diff/diff.cpp index 6daed321d11..70dd329e9ca 100644 --- a/source/diff/diff.cpp +++ b/source/diff/diff.cpp @@ -338,6 +338,30 @@ class Differ { std::function 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 + 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, @@ -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 + match_group) { + // Group the ids based on a key (get_group) + std::map src_groups; + std::map dst_groups; + + GroupIds(src_ids, true, &src_groups, get_group); + GroupIds(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; @@ -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); @@ -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( - 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) { @@ -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]); } }); } diff --git a/test/diff/diff_files/different_decorations_fragment_autogen.cpp b/test/diff/diff_files/different_decorations_fragment_autogen.cpp index 0d34654f840..ec9074c6404 100644 --- a/test/diff/diff_files/different_decorations_fragment_autogen.cpp +++ b/test/diff/diff_files/different_decorations_fragment_autogen.cpp @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 diff --git a/test/diff/diff_files/different_decorations_vertex_autogen.cpp b/test/diff/diff_files/different_decorations_vertex_autogen.cpp index f65ee5a1984..134ebb4a54d 100644 --- a/test/diff/diff_files/different_decorations_vertex_autogen.cpp +++ b/test/diff/diff_files/different_decorations_vertex_autogen.cpp @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 )"; diff --git a/test/diff/diff_files/different_function_parameter_count_autogen.cpp b/test/diff/diff_files/different_function_parameter_count_autogen.cpp index 3a077fb0ebf..0eaca958a08 100644 --- a/test/diff/diff_files/different_function_parameter_count_autogen.cpp +++ b/test/diff/diff_files/different_function_parameter_count_autogen.cpp @@ -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" @@ -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;