Reland "Ensure marking phi root as escaping if operand is escaping"
This reverts commit 4b8928aecee9c51aa41de88f3d465317882db798.
Bug: b/280958704
Change-Id: I9783be336e733131f7b0cc0d52e9d205e1c88826
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..dafdf5f 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
@@ -31,7 +31,6 @@
import com.android.tools.r8.ir.code.Phi;
import com.android.tools.r8.ir.code.Value;
import com.android.tools.r8.ir.optimize.string.StringBuilderNode.AppendNode;
-import com.android.tools.r8.ir.optimize.string.StringBuilderNode.EscapeNode;
import com.android.tools.r8.ir.optimize.string.StringBuilderNode.ImplicitToStringNode;
import com.android.tools.r8.ir.optimize.string.StringBuilderNode.InitNode;
import com.android.tools.r8.ir.optimize.string.StringBuilderNode.InitOrAppendNode;
@@ -177,15 +176,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, createEscapeNode()));
+ if (seenEscaped) {
+ addNodeToRootAndTail(currentRoots, currentTails, phi, createEscapeNode());
+ }
}
}
for (Instruction instruction : block.getInstructions()) {
@@ -198,16 +199,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 +212,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 +234,8 @@
return !newNode.isMutateNode() && !newNode.isInspectingNode();
} else if (insertedNode.isInspectingNode()) {
return !newNode.isInspectingNode();
+ } else if (insertedNode.isEscapeNode()) {
+ return !newNode.isEscapeNode();
}
return true;
}
@@ -461,8 +471,10 @@
}
assert currentTail != null;
// Link next node from successor
- currentTail.addSuccessor(sbNode);
- sbNode.addPredecessor(currentTail);
+ if (childStates.size() != 1 || shouldAddNodeToGraph(currentTail, sbNode)) {
+ currentTail.addSuccessor(sbNode);
+ sbNode.addPredecessor(currentTail);
+ }
});
if (childState.seenAndNotProcessed()) {
childGraphState.isPartOfLoop = 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 {
