Account for StringBuilder#toString as a separate builder state.
Otherwise, as demonstrated at b/147718617, a dominant state that has
conditional path with append() and another conditional path with
toString() could be regarded as a state with a single call chain,
resulting in incorrect compile-time optimization of builder.
Bug: 147718617
Change-Id: I0cc2f4400a7aa10be53c960dfbf44296e92774e7
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderOptimizer.java b/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderOptimizer.java
index 02b6e64..8e8d2da 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderOptimizer.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderOptimizer.java
@@ -415,7 +415,10 @@
Map<Instruction, BuilderState> perInstrState = getBuilderState(builder);
BuilderState dominantState = findDominantState(dominatorTree, perInstrState, instr);
if (dominantState != null) {
- perInstrState.put(instr, dominantState);
+ // Instead of using the dominant state directly, treat this retrieval point as a new
+ // state without an addition so that dominant state can account for dependent states.
+ BuilderState currentState = dominantState.createChild("");
+ perInstrState.put(instr, currentState);
} else {
// TODO(b/114002137): if we want to utilize partial results, don't remove it here.
candidateBuilders.remove(builder);
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/string/StringBuilderOptimizerAnalysisTest.java b/src/test/java/com/android/tools/r8/ir/optimize/string/StringBuilderOptimizerAnalysisTest.java
index f186803..e40b4e0 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/string/StringBuilderOptimizerAnalysisTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/string/StringBuilderOptimizerAnalysisTest.java
@@ -225,6 +225,21 @@
}
@Test
+ public void testConditionalPhiWithoutAppend() throws Exception {
+ buildAndCheckIR(
+ "conditionalPhiWithoutAppend",
+ checkOptimizerStates(appView, optimizer -> {
+ assertEquals(1, optimizer.analysis.builderStates.size());
+ for (Value builder : optimizer.analysis.builderStates.keySet()) {
+ Map<Instruction, BuilderState> perBuilderState =
+ optimizer.analysis.builderStates.get(builder);
+ checkBuilderState(optimizer, perBuilderState, null, true);
+ }
+ assertEquals(0, optimizer.analysis.simplifiedBuilders.size());
+ }));
+ }
+
+ @Test
public void testLoop() throws Exception {
buildAndCheckIR(
"loop",
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatenationTest.java b/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatenationTest.java
index 1b16aa6..0efe652 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatenationTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatenationTest.java
@@ -51,6 +51,8 @@
"Hello,R8",
// phiWithDifferentInits
"Hello,R8",
+ // conditionalPhiWithoutAppend
+ "initial:suffix",
// loop
"na;na;na;na;na;na;na;na;Batman!",
// loopWithBuilder
@@ -59,7 +61,7 @@
@Parameterized.Parameters(name = "{0}")
public static TestParametersCollection data() {
- return getTestParameters().withAllRuntimes().build();
+ return getTestParameters().withAllRuntimes().withAllApiLevels().build();
}
private final TestParameters parameters;
@@ -152,6 +154,10 @@
assertThat(method, isPresent());
assertEquals(3, countConstString(method));
+ method = mainClass.uniqueMethodWithName("conditionalPhiWithoutAppend");
+ assertThat(method, isPresent());
+ assertEquals(3, countConstString(method));
+
method = mainClass.uniqueMethodWithName("loop");
assertThat(method, isPresent());
assertEquals(3, countConstString(method));
@@ -173,7 +179,7 @@
testForD8()
.debug()
.addProgramClasses(MAIN)
- .setMinApi(parameters.getRuntime())
+ .setMinApi(parameters.getApiLevel())
.run(parameters.getRuntime(), MAIN)
.assertSuccessWithOutput(JAVA_OUTPUT);
test(result, false, false);
@@ -182,7 +188,7 @@
testForD8()
.release()
.addProgramClasses(MAIN)
- .setMinApi(parameters.getRuntime())
+ .setMinApi(parameters.getApiLevel())
.run(parameters.getRuntime(), MAIN)
.assertSuccessWithOutput(JAVA_OUTPUT);
test(result, false, true);
@@ -198,7 +204,7 @@
.enableInliningAnnotations()
.addKeepMainRule(MAIN)
.noMinification()
- .setMinApi(parameters.getRuntime())
+ .setMinApi(parameters.getApiLevel())
.run(parameters.getRuntime(), MAIN)
.assertSuccessWithOutput(JAVA_OUTPUT);
test(result, true, true);
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatenationTestClass.java b/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatenationTestClass.java
index 980148b..535fee9 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatenationTestClass.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatenationTestClass.java
@@ -138,6 +138,16 @@
}
@NeverInline
+ public static void conditionalPhiWithoutAppend() {
+ StringBuilder b = new StringBuilder("initial");
+ String suffix = "suffix";
+ if (!suffix.isEmpty()) {
+ b.append(":").append(suffix);
+ }
+ System.out.println(b.toString());
+ }
+
+ @NeverInline
public static void loop() {
String r = "";
for (int i = 0; i < 8; i++) {
@@ -169,6 +179,7 @@
simplePhi();
phiAtInit();
phiWithDifferentInits();
+ conditionalPhiWithoutAppend();
loop();
loopWithBuilder();
}