update_test_checks: improve IR value name stability #110940
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This test illustrates a surprising failure to keep IR value names stable. Change-Id: I242fba6668971177919dfe4092e6bd16e0d83c13
By default, UTC attempts to keep the produced diff small by keeping IR value name variables stable. The old algorithm was roughly: 1. Compute a diff between the old and new check lines, where "uncommitted" variable names are replaced by a wildcard. This leads to a set of non-crossing "candidate" pairs of (old line, new line) that we can try to make equal. 2. Greedily walk this list of candidates, committing to variable names that make candidate lines equal if possible. The greedy approach in the second step has the downside that committing to a variable name greedily can sometimes prevent many subsequent candidates from getting the variable name assignment that would make them equal. We keep the first step as-is, but replace the second one by an algorithm that finds a large independent set of candidates, i.e. candidate pairs of (old line, new line) which are non-conflicting in the sense that their desired variable name mappings are not in conflict. We find the large independent set by greedily assigning a coloring to the conflict graph and taking the largest color class. We then commit to all the variable name mappings which are desired by candidates in this largest color class. As before, we then recurse into regions between matching lines. This is required in large cases. For example, running this algorithm at the top-level of the new test case (stable_ir_values5.ll) matches up most of the instructions, but not the names of the result values of all the `load`s. This is because (unlike e.g. the getelementptrs) the load instructions are all equal except for variable names, and so step 1 (the diff algorithm) doesn't consider them as candidates. However, they are trivially matched by recursion. As is usually the case with these changes, the quality improvement is hard to see from the diff of this patch. However, it becomes obvious when comparing the diff of stable_ir_values5.ll against stable_ir_value5.ll.expected before and after this change.
|
@llvm/pr-subscribers-testing-tools Author: Nicolai Hähnle (nhaehnle) ChangesBy default, UTC attempts to keep the produced diff small by keeping IR
The greedy approach in the second step has the downside that committing We keep the first step as-is, but replace the second one by an algorithm We find the large independent set by greedily assigning a coloring to As before, we then recurse into regions between matching lines. This is This also happens to fix a bug in tracking line indices that went unnoticed previously... As is usually the case with these changes, the quality improvement is Patch is 45.28 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/110940.diff 4 Files Affected:
diff --git a/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/stable_ir_values5.ll b/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/stable_ir_values5.ll
new file mode 100644
index 00000000000000..4d503e36350fe2
--- /dev/null
+++ b/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/stable_ir_values5.ll
@@ -0,0 +1,305 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 3
+; RUN: opt < %s -S | FileCheck %s
+
+; Test a very long run of similar (but different) instructions in which the optimal
+; diff is sensitive to how names are mapped. This is already slightly reduced
+; from the original, but attempting to reduce it further ended up hiding another
+; bug.
+
+%structA = type { <3 x i32> }
+%structC = type { [50 x i32] }
+
+define dso_local void @main.resume.0(i64 %0, %structA %arg01, [23 x i32] %arg12, [30 x i32] %arg23) {
+; CHECK-LABEL: define dso_local void @main.resume.0(
+; CHECK-SAME: i64 [[TMP0:%.*]], { [[STRUCTA:%.*]], [23 x i32], [30 x i32] } [[TMP1:%.*]]) {
+; CHECK-NEXT: entryresume.0:
+; CHECK-NEXT: [[FOO:%.*]] = call ptr @getter(i32 108)
+; CHECK-NEXT: [[TMP3:%.*]] = extractvalue { [[STRUCTA]], [23 x i32], [30 x i32] } [[TMP1]], 2
+; CHECK-NEXT: [[DOTFCA_0_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 0
+; CHECK-NEXT: [[DOTFCA_1_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 1
+; CHECK-NEXT: [[DOTFCA_2_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 2
+; CHECK-NEXT: [[DOTFCA_3_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 3
+; CHECK-NEXT: [[DOTFCA_4_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 4
+; CHECK-NEXT: [[DOTFCA_5_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 5
+; CHECK-NEXT: [[DOTFCA_6_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 6
+; CHECK-NEXT: [[DOTFCA_7_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 7
+; CHECK-NEXT: [[DOTFCA_8_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 8
+; CHECK-NEXT: [[DOTFCA_9_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 9
+; CHECK-NEXT: [[DOTFCA_10_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 10
+; CHECK-NEXT: [[DOTFCA_11_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 11
+; CHECK-NEXT: [[DOTFCA_12_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 12
+; CHECK-NEXT: [[DOTFCA_13_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 13
+; CHECK-NEXT: [[DOTFCA_14_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 14
+; CHECK-NEXT: [[DOTFCA_15_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 15
+; CHECK-NEXT: [[DOTFCA_16_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 16
+; CHECK-NEXT: [[DOTFCA_17_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 17
+; CHECK-NEXT: [[DOTFCA_18_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 18
+; CHECK-NEXT: [[DOTFCA_19_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 19
+; CHECK-NEXT: [[DOTFCA_20_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 20
+; CHECK-NEXT: [[DOTFCA_21_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 21
+; CHECK-NEXT: [[DOTFCA_22_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 22
+; CHECK-NEXT: [[DOTFCA_23_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 23
+; CHECK-NEXT: [[DOTFCA_24_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 24
+; CHECK-NEXT: [[DOTFCA_25_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 25
+; CHECK-NEXT: [[DOTFCA_26_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 26
+; CHECK-NEXT: [[DOTFCA_27_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 27
+; CHECK-NEXT: [[TMP23:%.*]] = extractvalue [30 x i32] [[TMP3]], 28
+; CHECK-NEXT: [[DOTFCA_29_EXTRACT:%.*]] = extractvalue [30 x i32] [[TMP3]], 29
+; CHECK-NEXT: [[TMP4:%.*]] = freeze [[STRUCTC:%.*]] poison
+; CHECK-NEXT: [[DOTFCA_0_0_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 0
+; CHECK-NEXT: [[DOTFCA_0_1_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 1
+; CHECK-NEXT: [[DOTFCA_0_2_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 2
+; CHECK-NEXT: [[DOTFCA_0_3_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 3
+; CHECK-NEXT: [[DOTFCA_0_4_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 4
+; CHECK-NEXT: [[DOTFCA_0_5_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 5
+; CHECK-NEXT: [[DOTFCA_0_6_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 6
+; CHECK-NEXT: [[DOTFCA_0_7_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 7
+; CHECK-NEXT: [[DOTFCA_0_8_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 8
+; CHECK-NEXT: [[DOTFCA_0_9_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 9
+; CHECK-NEXT: [[DOTFCA_0_10_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 10
+; CHECK-NEXT: [[DOTFCA_0_11_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 11
+; CHECK-NEXT: [[DOTFCA_0_12_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 12
+; CHECK-NEXT: [[DOTFCA_0_13_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 13
+; CHECK-NEXT: [[DOTFCA_0_14_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 14
+; CHECK-NEXT: [[DOTFCA_0_15_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 15
+; CHECK-NEXT: [[DOTFCA_0_16_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 16
+; CHECK-NEXT: [[DOTFCA_0_17_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 17
+; CHECK-NEXT: [[DOTFCA_0_18_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 18
+; CHECK-NEXT: [[DOTFCA_0_19_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 19
+; CHECK-NEXT: [[DOTFCA_0_20_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 20
+; CHECK-NEXT: [[DOTFCA_0_21_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 21
+; CHECK-NEXT: [[DOTFCA_0_22_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 22
+; CHECK-NEXT: [[DOTFCA_0_23_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 23
+; CHECK-NEXT: [[DOTFCA_0_24_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 24
+; CHECK-NEXT: [[DOTFCA_0_25_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 25
+; CHECK-NEXT: [[DOTFCA_0_26_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 26
+; CHECK-NEXT: [[DOTFCA_0_27_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 27
+; CHECK-NEXT: [[DOTFCA_0_28_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 28
+; CHECK-NEXT: [[DOTFCA_0_29_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 29
+; CHECK-NEXT: [[DOTFCA_0_30_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 30
+; CHECK-NEXT: [[DOTFCA_0_31_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 31
+; CHECK-NEXT: [[DOTFCA_0_32_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 32
+; CHECK-NEXT: [[DOTFCA_0_33_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 33
+; CHECK-NEXT: [[DOTFCA_0_34_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 34
+; CHECK-NEXT: [[DOTFCA_0_35_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 35
+; CHECK-NEXT: [[DOTFCA_0_36_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 36
+; CHECK-NEXT: [[DOTFCA_0_37_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 37
+; CHECK-NEXT: [[DOTFCA_0_38_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 38
+; CHECK-NEXT: [[DOTFCA_0_39_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 39
+; CHECK-NEXT: [[DOTFCA_0_40_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 40
+; CHECK-NEXT: [[DOTFCA_0_41_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 41
+; CHECK-NEXT: [[DOTFCA_0_42_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 42
+; CHECK-NEXT: [[DOTFCA_0_43_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 43
+; CHECK-NEXT: [[DOTFCA_0_44_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 44
+; CHECK-NEXT: [[DOTFCA_0_45_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 45
+; CHECK-NEXT: [[DOTFCA_0_46_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 46
+; CHECK-NEXT: [[DOTFCA_0_47_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 47
+; CHECK-NEXT: [[DOTFCA_0_48_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 48
+; CHECK-NEXT: [[DOTFCA_0_49_EXTRACT:%.*]] = extractvalue [[STRUCTC]] [[TMP4]], 0, 49
+; CHECK-NEXT: [[TMP2:%.*]] = inttoptr i32 [[DOTFCA_0_EXTRACT]] to ptr
+; CHECK-NEXT: [[TMP5:%.*]] = load i32, ptr [[TMP2]], align 4
+; CHECK-NEXT: [[TMP27:%.*]] = getelementptr inbounds i32, ptr [[TMP2]], i32 1
+; CHECK-NEXT: [[TMP7:%.*]] = load i32, ptr [[TMP27]], align 4
+; CHECK-NEXT: [[TMP29:%.*]] = getelementptr inbounds i32, ptr [[TMP2]], i32 2
+; CHECK-NEXT: [[TMP9:%.*]] = load i32, ptr [[TMP29]], align 4
+; CHECK-NEXT: [[TMP31:%.*]] = getelementptr inbounds i32, ptr [[TMP2]], i32 3
+; CHECK-NEXT: [[TMP11:%.*]] = load i32, ptr [[TMP31]], align 4
+; CHECK-NEXT: [[TMP33:%.*]] = getelementptr inbounds i32, ptr [[TMP2]], i32 4
+; CHECK-NEXT: [[TMP13:%.*]] = load i32, ptr [[TMP33]], align 4
+; CHECK-NEXT: [[TMP35:%.*]] = getelementptr inbounds i32, ptr [[TMP2]], i32 5
+; CHECK-NEXT: [[TMP15:%.*]] = load i32, ptr [[TMP35]], align 4
+; CHECK-NEXT: [[TMP37:%.*]] = getelementptr inbounds i32, ptr [[TMP2]], i32 6
+; CHECK-NEXT: [[TMP17:%.*]] = load i32, ptr [[TMP37]], align 4
+; CHECK-NEXT: [[TMP39:%.*]] = getelementptr inbounds i32, ptr [[TMP2]], i32 7
+; CHECK-NEXT: [[TMP19:%.*]] = load i32, ptr [[TMP39]], align 4
+; CHECK-NEXT: [[TMP41:%.*]] = getelementptr inbounds i32, ptr [[TMP2]], i32 8
+; CHECK-NEXT: [[TMP21:%.*]] = load i32, ptr [[TMP41]], align 4
+; CHECK-NEXT: [[TMP43:%.*]] = getelementptr inbounds i32, ptr [[TMP2]], i32 9
+; CHECK-NEXT: [[TMP24:%.*]] = load i32, ptr [[TMP43]], align 4
+; CHECK-NEXT: [[TMP45:%.*]] = getelementptr inbounds i32, ptr [[TMP2]], i32 10
+; CHECK-NEXT: [[TMP25:%.*]] = load i32, ptr [[TMP45]], align 4
+; CHECK-NEXT: [[TMP47:%.*]] = getelementptr inbounds i32, ptr [[TMP2]], i32 11
+; CHECK-NEXT: [[TMP28:%.*]] = load i32, ptr [[TMP47]], align 4
+; CHECK-NEXT: [[TMP49:%.*]] = getelementptr inbounds i32, ptr [[TMP2]], i32 12
+; CHECK-NEXT: [[TMP30:%.*]] = load i32, ptr [[TMP49]], align 4
+; CHECK-NEXT: [[TMP51:%.*]] = getelementptr inbounds i32, ptr [[TMP2]], i32 13
+; CHECK-NEXT: [[TMP32:%.*]] = load i32, ptr [[TMP51]], align 4
+; CHECK-NEXT: [[TMP53:%.*]] = getelementptr inbounds i32, ptr [[TMP2]], i32 14
+; CHECK-NEXT: [[TMP34:%.*]] = load i32, ptr [[TMP53]], align 4
+; CHECK-NEXT: [[TMP55:%.*]] = getelementptr inbounds i32, ptr [[TMP2]], i32 15
+; CHECK-NEXT: [[TMP36:%.*]] = load i32, ptr [[TMP55]], align 4
+; CHECK-NEXT: [[TMP57:%.*]] = getelementptr inbounds i32, ptr [[TMP2]], i32 16
+; CHECK-NEXT: [[TMP38:%.*]] = load i32, ptr [[TMP57]], align 4
+; CHECK-NEXT: [[TMP59:%.*]] = getelementptr inbounds i32, ptr [[TMP2]], i32 17
+; CHECK-NEXT: [[TMP40:%.*]] = load i32, ptr [[TMP59]], align 4
+; CHECK-NEXT: [[TMP61:%.*]] = getelementptr inbounds i32, ptr [[TMP2]], i32 18
+; CHECK-NEXT: [[TMP42:%.*]] = load i32, ptr [[TMP61]], align 4
+; CHECK-NEXT: [[TMP63:%.*]] = getelementptr inbounds i32, ptr [[TMP2]], i32 19
+; CHECK-NEXT: [[TMP44:%.*]] = load i32, ptr [[TMP63]], align 4
+; CHECK-NEXT: [[TMP65:%.*]] = getelementptr inbounds i32, ptr [[TMP2]], i32 20
+; CHECK-NEXT: [[TMP46:%.*]] = load i32, ptr [[TMP65]], align 4
+; CHECK-NEXT: [[TMP67:%.*]] = getelementptr inbounds i32, ptr [[TMP2]], i32 21
+; CHECK-NEXT: [[TMP48:%.*]] = load i32, ptr [[TMP67]], align 4
+; CHECK-NEXT: [[TMP69:%.*]] = getelementptr inbounds i32, ptr [[TMP2]], i32 22
+; CHECK-NEXT: [[TMP50:%.*]] = load i32, ptr [[TMP69]], align 4
+; CHECK-NEXT: [[TMP71:%.*]] = getelementptr inbounds i32, ptr [[TMP2]], i32 23
+; CHECK-NEXT: [[TMP52:%.*]] = load i32, ptr [[TMP71]], align 4
+; CHECK-NEXT: [[TMP73:%.*]] = getelementptr inbounds i32, ptr [[TMP2]], i32 24
+; CHECK-NEXT: [[TMP54:%.*]] = load i32, ptr [[TMP73]], align 4
+; CHECK-NEXT: [[TMP75:%.*]] = getelementptr inbounds i32, ptr [[TMP2]], i32 25
+; CHECK-NEXT: [[TMP56:%.*]] = load i32, ptr [[TMP75]], align 4
+; CHECK-NEXT: [[TMP77:%.*]] = getelementptr inbounds i32, ptr [[TMP2]], i32 26
+; CHECK-NEXT: [[TMP62:%.*]] = load i32, ptr [[TMP77]], align 4
+; CHECK-NEXT: [[TMP60:%.*]] = inttoptr i32 [[DOTFCA_0_EXTRACT]] to ptr
+; CHECK-NEXT: [[TMP58:%.*]] = extractvalue { [[STRUCTA]], [23 x i32], [30 x i32] } [[TMP1]], 0
+; CHECK-NEXT: [[DOTFCA_0_EXTRACT57:%.*]] = extractvalue [[STRUCTA]] [[TMP58]], 0
+; CHECK-NEXT: ret void
+;
+entryresume.0:
+ %1 = call ptr @getter(i32 108)
+ %2 = insertvalue { %structA, [23 x i32], [30 x i32] } poison, %structA %arg01, 0
+ %3 = insertvalue { %structA, [23 x i32], [30 x i32] } %2, [23 x i32] %arg12, 1
+ %4 = insertvalue { %structA, [23 x i32], [30 x i32] } %3, [30 x i32] %arg23, 2
+ %5 = extractvalue { %structA, [23 x i32], [30 x i32] } %4, 2
+ %.fca.0.extract = extractvalue [30 x i32] %5, 0
+ %.fca.1.extract = extractvalue [30 x i32] %5, 1
+ %.fca.2.extract = extractvalue [30 x i32] %5, 2
+ %.fca.3.extract = extractvalue [30 x i32] %5, 3
+ %.fca.4.extract = extractvalue [30 x i32] %5, 4
+ %.fca.5.extract = extractvalue [30 x i32] %5, 5
+ %.fca.6.extract = extractvalue [30 x i32] %5, 6
+ %.fca.7.extract = extractvalue [30 x i32] %5, 7
+ %.fca.8.extract = extractvalue [30 x i32] %5, 8
+ %.fca.9.extract = extractvalue [30 x i32] %5, 9
+ %.fca.10.extract = extractvalue [30 x i32] %5, 10
+ %.fca.11.extract = extractvalue [30 x i32] %5, 11
+ %.fca.12.extract = extractvalue [30 x i32] %5, 12
+ %.fca.13.extract = extractvalue [30 x i32] %5, 13
+ %.fca.14.extract = extractvalue [30 x i32] %5, 14
+ %.fca.15.extract = extractvalue [30 x i32] %5, 15
+ %.fca.16.extract = extractvalue [30 x i32] %5, 16
+ %.fca.17.extract = extractvalue [30 x i32] %5, 17
+ %.fca.18.extract = extractvalue [30 x i32] %5, 18
+ %.fca.19.extract = extractvalue [30 x i32] %5, 19
+ %.fca.20.extract = extractvalue [30 x i32] %5, 20
+ %.fca.21.extract = extractvalue [30 x i32] %5, 21
+ %.fca.22.extract = extractvalue [30 x i32] %5, 22
+ %.fca.23.extract = extractvalue [30 x i32] %5, 23
+ %.fca.24.extract = extractvalue [30 x i32] %5, 24
+ %.fca.25.extract = extractvalue [30 x i32] %5, 25
+ %.fca.26.extract = extractvalue [30 x i32] %5, 26
+ %.fca.27.extract = extractvalue [30 x i32] %5, 27
+ %.fca.28.extract = extractvalue [30 x i32] %5, 28
+ %.fca.29.extract = extractvalue [30 x i32] %5, 29
+ %6 = freeze %structC poison
+ %.fca.0.0.extract = extractvalue %structC %6, 0, 0
+ %.fca.0.1.extract = extractvalue %structC %6, 0, 1
+ %.fca.0.2.extract = extractvalue %structC %6, 0, 2
+ %.fca.0.3.extract = extractvalue %structC %6, 0, 3
+ %.fca.0.4.extract = extractvalue %structC %6, 0, 4
+ %.fca.0.5.extract = extractvalue %structC %6, 0, 5
+ %.fca.0.6.extract = extractvalue %structC %6, 0, 6
+ %.fca.0.7.extract = extractvalue %structC %6, 0, 7
+ %.fca.0.8.extract = extractvalue %structC %6, 0, 8
+ %.fca.0.9.extract = extractvalue %structC %6, 0, 9
+ %.fca.0.10.extract = extractvalue %structC %6, 0, 10
+ %.fca.0.11.extract = extractvalue %structC %6, 0, 11
+ %.fca.0.12.extract = extractvalue %structC %6, 0, 12
+ %.fca.0.13.extract = extractvalue %structC %6, 0, 13
+ %.fca.0.14.extract = extractvalue %structC %6, 0, 14
+ %.fca.0.15.extract = extractvalue %structC %6, 0, 15
+ %.fca.0.16.extract = extractvalue %structC %6, 0, 16
+ %.fca.0.17.extract = extractvalue %structC %6, 0, 17
+ %.fca.0.18.extract = extractvalue %structC %6, 0, 18
+ %.fca.0.19.extract = extractvalue %structC %6, 0, 19
+ %.fca.0.20.extract = extractvalue %structC %6, 0, 20
+ %.fca.0.21.extract = extractvalue %structC %6, 0, 21
+ %.fca.0.22.extract = extractvalue %structC %6, 0, 22
+ %.fca.0.23.extract = extractvalue %structC %6, 0, 23
+ %.fca.0.24.extract = extractvalue %structC %6, 0, 24
+ %.fca.0.25.extract = extractvalue %structC %6, 0, 25
+ %.fca.0.26.extract = extractvalue %structC %6, 0, 26
+ %.fca.0.27.extract = extractvalue %structC %6, 0, 27
+ %.fca.0.28.extract = extractvalue %structC %6, 0, 28
+ %.fca.0.29.extract = extractvalue %structC %6, 0, 29
+ %.fca.0.30.extract = extractvalue %structC %6, 0, 30
+ %.fca.0.31.extract = extractvalue %structC %6, 0, 31
+ %.fca.0.32.extract = extractvalue %structC %6, 0, 32
+ %.fca.0.33.extract = extractvalue %structC %6, 0, 33
+ %.fca.0.34.extract = extractvalue %structC %6, 0, 34
+ %.fca.0.35.extract = extractvalue %structC %6, 0, 35
+ %.fca.0.36.extract = extractvalue %structC %6, 0, 36
+ %.fca.0.37.extract = extractvalue %structC %6, 0, 37
+ %.fca.0.38.extract = extractvalue %structC %6, 0, 38
+ %.fca.0.39.extract = extractvalue %structC %6, 0, 39
+ %.fca.0.40.extract = extractvalue %structC %6, 0, 40
+ %.fca.0.41.extract = extractvalue %structC %6, 0, 41
+ %.fca.0.42.extract = extractvalue %structC %6, 0, 42
+ %.fca.0.43.extract = extractvalue %structC %6, 0, 43
+ %.fca.0.44.extract = extractvalue %structC %6, 0, 44
+ %.fca.0.45.extract = extractvalue %structC %6, 0, 45
+ %.fca.0.46.extract = extractvalue %structC %6, 0, 46
+ %.fca.0.47.extract = extractvalue %structC %6, 0, 47
+ %.fca.0.48.extract = extractvalue %structC %6, 0, 48
+ %.fca.0.49.extract = extractvalue %structC %6, 0, 49
+ %7 = inttoptr i32 %.fca.0.extract to ptr
+ %8 = load i32, ptr %7, align 4
+ %9 = getelementptr inbounds i32, ptr %7, i32 1
+ %10 = load i32, ptr %9, align 4
+ %11 = getelementptr inbounds i32, ptr %7, i32 2
+ %12 = load i32, ptr %11, align 4
+ %13 = getelementptr inbounds i32, ptr %7, i32 3
+ %14 = load i32, ptr %13, align 4
+ %15 = getelementptr inbounds i32, ptr %7, i32 4
+ %16 = load i32, ptr %15, align 4
+ %17 = getelementptr inbounds i32, ptr %7, i32 5
+ %18 = load i32, ptr %17, align 4
+ %19 = getelementptr inbounds i32, ptr %7, i32 6
+ %20 = load i32, ptr %19, align 4
+ %21 = getelementptr inbounds i32, ptr %7, i32 7
+ %22 = load i32, ptr %21, align 4
+ %23 = getelementptr inbounds i32, ptr %7, i32 8
+ %24 = load i32, ptr %23, align 4
+ %25 = getelementptr inbounds i32, ptr %7, i32 9
+ %26 = load i32, ptr %25, align 4
+ %27 = getelementptr inbounds i32, ptr %7, i32 10
+ %28 = load i32, ptr %27, align 4
+ %29 = getelementptr inbounds i32, ptr %7, i32 11
+ %30 = load i32, ptr %29, align 4
+ %31 = getelementptr inbounds i32, ptr %7, i32 12
+ %32 = load i32, ptr %31, align 4
+ %33 = getelementptr inbounds i32, ptr %7, i32 13
+ %34 = load i32, ptr %33, align 4
+ %35 = getelementptr inbounds i32, ptr %7, i32 14
+ %36 = load i32, ptr %35, align 4
+ %37 = getelementptr inbounds i32, ptr %7, i32 15
+ %38 = load i32, ptr %37, align 4
+ %39 = getelementptr inbounds i32, ptr %7, i32 16
+ %40 = load i32, ptr %39, align 4
+ %41 = getelementptr inbounds i32, ptr %7, i32 17
+ %42 = load i32, ptr %41, align 4
+ %43 = getelementptr inbounds i32, ptr %7, i32 18
+ %44 = load i32, ptr %43, align 4
+ %45 = getelementptr inbounds i32, ptr %7, i32 19
+ %46 = load i32, ptr %45, align 4
+ %47 = getelementptr inbounds i32, ptr %7, i32 20
+ %48 = load i32, ptr %47, align 4
+ %49 = getelementptr inbounds i32, ptr %7, i32 21
+ %50 = load i32, ptr %49, align 4
+ %51 = getelementptr inbounds i32, ptr %7, i32 22
+ %52 = load i32, ptr %51, align 4
+ %53 = getelementptr inbounds i32, ptr %7, i32 23
+ %54 = load i32, ptr %53, align 4
+ %55 = getelementptr inbounds i32, ptr %7, i32 24
+ %56 = load i32, ptr %55, align 4
+ %57 = getelementptr inbounds i32, ptr %7, i32 25
+ %58 = load i32, ptr %57, align 4
+ %59 = getelementptr inbounds i32, ptr %7, i32 26
+ %60 = load i32, ptr %59, align 4
+ %61 = inttoptr i32 %.fca.0.extract to ptr
+ %62 = extractvalue { %structA, [23 x i32], [30 x i32] } %4, 0
+ %.fca.0.extract57 = extractvalue %structA %62, 0
+ ret void
+}
+
+declare ptr @getter(i32)
diff --git a/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/stable_ir_values5.ll.expected b/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/stable_ir_values5.ll.expected
new file mode 100644
index 00000000000000..40bf55e70a9588
--- /dev/null
+++ b/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/stable_ir_values5.ll.expected
@@ -0,0 +1,308 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 3
+; RUN: opt < %s -S | FileCheck %s
+
+; Test a very long run of similar (but different) instructions in which the optimal
+; diff is sensitive to how names are mapped. This is already slightl...
[truncated]
|
|
✅ With the latest revision this PR passed the Python code formatter. |
FWIW I don't think this functionality actually works in practice. I get the same amount of "rename diffs" before and after this feature was introduced. Does it only work for some specific UTC |
jasilvanus
reviewed
Oct 4, 2024
There was a problem hiding this comment.
Thanks for this improvement!
FWIW I don't think this functionality actually works in practice. I get the same amount of "rename diffs" before and after this feature was introduced. Does it only work for some specific UTC --version and not any of the older tests, or something like that?
For me it mostly worked nicely in practice, and sometimes failed (sometimes even introducing extra diff), which I suspect to be caused by the issue improved in this PR. I think it's always enabled by default independent of the version, but maybe only for IR tests?
The diff on the new test looks good.
For convenience, this is how the diff on stable_ir_values5.ll would have appeared in a PR:
Before: c7e1732
After: f24b14b
Maybe we should put in a comment that briefly explains the idea of the algorithm? Something like:
# We want to determine a large set of compatible candidates, because this leads
# to a small diff. This is equivalent to interpreting the candidates as vertices in a
# conflict graph, adding edges between incompatible candidates, and searching
# for a large independent set.
# Greedily selecting candidates and removing incompatible ones has the disadvantage
# that making few bad decisions early on can have huge consequences.
# Instead, we compute multiple (as few as possible) independent sets, putting every
# candidate in exactly one of the independent sets. Then, we select the largest independent
# set found, commit to all candidates in it, and recurse.
# This is equivalent to finding a coloring of the conflict graph.
It really should work independent of the version and it has worked for me -- though clearly with some weaknesses which this change is meant to address :) The way to turn it off is via the --reset-variable-names option. If you have a concrete example where it fails, that would be interesting. |
|
@nhaehnle If you mass-regenerate InstCombine tests, you get ~100 files changed, where most of these changes are only instruction renames. llvm/test/Transforms/InstCombine/trunc-shift-trunc.ll would be a random example. |
jasilvanus
approved these changes
Oct 4, 2024
dtcxzyw
reviewed
Oct 5, 2024
| # Same, but for a possible commit happening on the same line | ||
| if local_commits[rhs_value.name] == lhs_value.name: | ||
| if new_color.color[rhs_value.name] == lhs_value.name: |
There was a problem hiding this comment.
I cannot update IR tests after this patch:
python3 ../../llvm-project/llvm/utils/update_test_checks.py --opt-binary=bin/opt ../../llvm-project/llvm/test/Transforms/InstCombine/add.ll
Traceback (most recent call last):
File "/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/../../llvm-project/llvm/utils/update_test_checks.py", line 370, in <module>
main()
File "/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/../../llvm-project/llvm/utils/update_test_checks.py", line 277, in main
common.add_ir_checks(
File "/home/dtcxzyw/WorkSpace/Projects/compilers/llvm-project/llvm/utils/UpdateTestChecks/common.py", line 2157, in add_ir_checks
return add_checks(
File "/home/dtcxzyw/WorkSpace/Projects/compilers/llvm-project/llvm/utils/UpdateTestChecks/common.py", line 2076, in add_checks
func_body = generalize_check_lines(
File "/home/dtcxzyw/WorkSpace/Projects/compilers/llvm-project/llvm/utils/UpdateTestChecks/common.py", line 1844, in generalize_check_lines
mapping = remap_metavar_names(orig_line_infos, new_line_infos, committed_names)
File "/home/dtcxzyw/WorkSpace/Projects/compilers/llvm-project/llvm/utils/UpdateTestChecks/common.py", line 1672, in remap_metavar_names
recurse(0, len(old_line_infos), 0, len(new_line_infos))
File "/home/dtcxzyw/WorkSpace/Projects/compilers/llvm-project/llvm/utils/UpdateTestChecks/common.py", line 1606, in recurse
if new_color.color[rhs_value.name] == lhs_value.name:
AttributeError: 'Color' object has no attribute 'color'
IIRC it should be new_color.mapping[rhs_value.name].
There was a problem hiding this comment.
You're right, thanks. #111347
nhaehnle
added a commit
that referenced
this pull request
Oct 7, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By default, UTC attempts to keep the produced diff small by keeping IR
value name variables stable. The old algorithm was roughly:
Compute a diff between the old and new check lines, where
"uncommitted" variable names are replaced by a wildcard.
This leads to a set of non-crossing "candidate" pairs of
(old line, new line) that we can try to make equal.
Greedily walk this list of candidates, committing to variable names
that make candidate lines equal if possible.
The greedy approach in the second step has the downside that committing
to a variable name greedily can sometimes prevent many subsequent
candidates from getting the variable name assignment that would make
them equal.
We keep the first step as-is, but replace the second one by an algorithm
that finds a large independent set of candidates, i.e. candidate pairs
of (old line, new line) which are non-conflicting in the sense that
their desired variable name mappings are not in conflict.
We find the large independent set by greedily assigning a coloring to
the conflict graph and taking the largest color class. We then commit to
all the variable name mappings which are desired by candidates in this
largest color class.
As before, we then recurse into regions between matching lines. This is
required in large cases. For example, running this algorithm at the
top-level of the new test case (stable_ir_values5.ll) matches up most of
the instructions, but not the names of the result values of all the
loads. This is because (unlike e.g. the getelementptrs) the loadinstructions are all equal except for variable names, and so step 1 (the
diff algorithm) doesn't consider them as candidates. However, they are
trivially matched by recursion.
This also happens to fix a bug in tracking line indices that went unnoticed previously...
As is usually the case with these changes, the quality improvement is
hard to see from the diff of this patch. However, it becomes obvious when
comparing the diff of stable_ir_values5.ll against stable_ir_value5.ll.expected
before and after this change.