Ensure marking phi root as escaping if operand is escaping
Bug: b/280958704
Change-Id: I2946f3e1dfac5e3de43459672321962eb53f345e
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderAppendOptimizer.java b/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderAppendOptimizer.java
index 5f0bea9..35ba7fb 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderAppendOptimizer.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderAppendOptimizer.java
@@ -177,15 +177,17 @@
previousState = result.asAbstractState();
for (Phi phi : block.getPhis()) {
if (previousState.isLiveStringBuilder(phi)) {
- visitAllAliasing(
- phi,
- previousState,
- value -> {},
- alias -> {
- EscapeNode escapeNode = new EscapeNode();
- currentRoots.put(alias, escapeNode);
- currentTails.put(alias, escapeNode);
- });
+ boolean seenEscaped =
+ visitAllAliasing(
+ phi,
+ previousState,
+ value -> {},
+ alias ->
+ addNodeToRootAndTail(
+ currentRoots, currentTails, alias, EscapeNode.empty()));
+ if (seenEscaped) {
+ addNodeToRootAndTail(currentRoots, currentTails, phi, EscapeNode.empty());
+ }
}
}
for (Instruction instruction : block.getInstructions()) {
@@ -198,16 +200,8 @@
createNodesForInstruction(
instruction,
previousState,
- (value, sbNode) -> {
- StringBuilderNode currentTail = currentTails.get(value);
- if (currentTail == null) {
- currentRoots.put(value, sbNode);
- currentTails.put(value, sbNode);
- } else if (shouldAddNodeToGraph(currentTail, sbNode)) {
- currentTail.addSuccessor(sbNode);
- currentTails.put(value, sbNode);
- }
- });
+ (value, sbNode) ->
+ addNodeToRootAndTail(currentRoots, currentTails, value, sbNode));
}
assert currentRoots.keySet().equals(currentTails.keySet());
assert previousState.getLiveStringBuilders().containsAll(currentRoots.keySet())
@@ -219,6 +213,21 @@
return TraversalContinuation.doContinue();
}
+ private void addNodeToRootAndTail(
+ Map<Value, StringBuilderNode> currentRoots,
+ Map<Value, StringBuilderNode> currentTails,
+ Value value,
+ StringBuilderNode node) {
+ StringBuilderNode currentTail = currentTails.get(value);
+ if (currentTail == null) {
+ currentRoots.put(value, node);
+ currentTails.put(value, node);
+ } else if (shouldAddNodeToGraph(currentTail, node)) {
+ currentTail.addSuccessor(node);
+ currentTails.put(value, node);
+ }
+ }
+
private boolean shouldAddNodeToGraph(
StringBuilderNode insertedNode, StringBuilderNode newNode) {
// No need for multiple mutating nodes or inspecting nodes.
@@ -226,6 +235,8 @@
return !newNode.isMutateNode() && !newNode.isInspectingNode();
} else if (insertedNode.isInspectingNode()) {
return !newNode.isInspectingNode();
+ } else if (insertedNode.isEscapeNode()) {
+ return !newNode.isEscapeNode();
}
return true;
}
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderNode.java b/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderNode.java
index 033a17c..be60f19 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderNode.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderNode.java
@@ -218,6 +218,12 @@
/** EscapeNode is used to explicitly define an escape point for a StringBuilder. */
static class EscapeNode extends StringBuilderNode {
+ private static EscapeNode EMPTY = new EscapeNode();
+
+ public static EscapeNode empty() {
+ return EMPTY;
+ }
+
@Override
boolean isEscapeNode() {
return true;
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/string/StringBuilderWithEscapingPhiOperandTest.java b/src/test/java/com/android/tools/r8/ir/optimize/string/StringBuilderWithEscapingPhiOperandTest.java
index 4c3e742..a7232eb 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/string/StringBuilderWithEscapingPhiOperandTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/string/StringBuilderWithEscapingPhiOperandTest.java
@@ -61,8 +61,7 @@
.release()
.setMinApi(parameters)
.run(parameters.getRuntime(), Main.class, "Hello World")
- // TODO(b/280958704): This should have same output as D8 debug/JVM.
- .assertSuccessWithOutputLines("(Hello World");
+ .assertSuccessWithOutputLines(EXPECTED);
}
@Test
@@ -80,8 +79,7 @@
assertThat(clazz.uniqueMethodWithOriginalName("storeInField"), isAbsent());
})
.run(parameters.getRuntime(), Main.class, "Hello World")
- // TODO(b/280958704): This should have same output as D8 debug/JVM.
- .assertSuccessWithOutputLines("(Hello World");
+ .assertSuccessWithOutputLines(EXPECTED);
}
static class Main {
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/string/StringBuilderWithEscapingPhiOperandThroughAssignSideEffectTest.java b/src/test/java/com/android/tools/r8/ir/optimize/string/StringBuilderWithEscapingPhiOperandThroughAssignSideEffectTest.java
index e5bcd85..c480ec5 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/string/StringBuilderWithEscapingPhiOperandThroughAssignSideEffectTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/string/StringBuilderWithEscapingPhiOperandThroughAssignSideEffectTest.java
@@ -56,8 +56,7 @@
.release()
.setMinApi(parameters)
.run(parameters.getRuntime(), Main.class, "Hello World")
- // TODO(b/280958704): This should have same output as D8 debug/JVM.
- .assertSuccessWithOutputLines("(Hello World");
+ .assertSuccessWithOutputLines(EXPECTED);
}
@Test
@@ -68,8 +67,7 @@
.enableInliningAnnotations()
.setMinApi(parameters)
.run(parameters.getRuntime(), Main.class, "Hello World")
- // TODO(b/280958704): This should have same output as D8 debug/JVM.
- .assertSuccessWithOutputLines("");
+ .assertSuccessWithOutputLines(EXPECTED);
}
static class Main {