Extend StringBuilder optimization to non-string type arguments.
Bug: 114002137
Change-Id: I1c9d355e98d4e63fce184b839ccdb9b1258a3edf
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 3ba841d..0b445a6 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
@@ -16,6 +16,7 @@
import com.android.tools.r8.ir.analysis.type.TypeLatticeElement;
import com.android.tools.r8.ir.code.BasicBlock;
import com.android.tools.r8.ir.code.BasicBlock.ThrowingInfo;
+import com.android.tools.r8.ir.code.ConstNumber;
import com.android.tools.r8.ir.code.ConstString;
import com.android.tools.r8.ir.code.DominatorTree;
import com.android.tools.r8.ir.code.DominatorTree.Assumption;
@@ -26,7 +27,9 @@
import com.android.tools.r8.ir.code.InvokeMethod;
import com.android.tools.r8.ir.code.InvokeMethodWithReceiver;
import com.android.tools.r8.ir.code.InvokeVirtual;
+import com.android.tools.r8.ir.code.NumberConversion;
import com.android.tools.r8.ir.code.Value;
+import com.android.tools.r8.ir.code.ValueType;
import com.android.tools.r8.logging.Log;
import com.android.tools.r8.utils.StringUtils;
import com.google.common.annotations.VisibleForTesting;
@@ -85,7 +88,7 @@
private int numberOfBuildersThatEscape = 0;
private int numberOfBuildersWhoseResultIsInterned = 0;
private int numberOfBuildersWithNonTrivialStateChange = 0;
- private int numberOfBuildersWithNonStringArg = 0;
+ private int numberOfBuildersWithUnsupportedArg = 0;
private int numberOfBuildersWithMergingPoints = 0;
private int numberOfBuildersWithNonDeterministicArg = 0;
private int numberOfDeadBuilders = 0;
@@ -126,7 +129,7 @@
Log.info(getClass(),
"# builders w/ non-trivial state change: %s", numberOfBuildersWithNonTrivialStateChange);
Log.info(getClass(),
- "# builders w/ non-string arg: %s", numberOfBuildersWithNonStringArg);
+ "# builders w/ unsupported arg: %s", numberOfBuildersWithUnsupportedArg);
Log.info(getClass(),
"# builders w/ merging points: %s", numberOfBuildersWithMergingPoints);
Log.info(getClass(),
@@ -354,8 +357,9 @@
continue;
}
assert invoke.inValues().size() == 2;
- Value arg = invoke.inValues().get(1);
- String addition = extractConstantArgument(arg);
+ Value arg = invoke.inValues().get(1).getAliasedValue();
+ DexType argType = invoke.getInvokedMethod().proto.parameters.values[0];
+ String addition = extractConstantArgument(arg, argType);
Map<Instruction, BuilderState> perInstrState = getBuilderState(builder);
BuilderState dominantState = findDominantState(dominatorTree, perInstrState, instr);
if (dominantState != null) {
@@ -380,8 +384,9 @@
if (!candidateBuilders.contains(builder)) {
continue;
}
- Value arg = invoke.inValues().get(1);
- String addition = extractConstantArgument(arg);
+ Value arg = invoke.inValues().get(1).getAliasedValue();
+ DexType argType = invoke.getInvokedMethod().proto.parameters.values[0];
+ String addition = extractConstantArgument(arg, argType);
Map<Instruction, BuilderState> perInstrState = getBuilderState(builder);
BuilderState dominantState = findDominantState(dominatorTree, perInstrState, instr);
if (dominantState != null) {
@@ -414,17 +419,86 @@
return this;
}
- private String extractConstantArgument(Value arg) {
+ private String extractConstantArgument(Value arg, DexType argType) {
String addition = ANY_STRING;
if (!arg.isPhi()) {
- // TODO(b/114002137): Improve arg extraction and type conversion.
if (arg.definition.isConstString()) {
addition = arg.definition.asConstString().getValue().toString();
+ } else if (arg.definition.isConstNumber() || arg.definition.isNumberConversion()) {
+ Number number = extractConstantNumber(arg);
+ if (number == null) {
+ return addition;
+ }
+ if (arg.getTypeLattice().isPrimitive()) {
+ if (argType == factory.booleanType) {
+ addition = String.valueOf(number.intValue() != 0);
+ } else if (argType == factory.byteType) {
+ addition = String.valueOf(number.byteValue());
+ } else if (argType == factory.shortType) {
+ addition = String.valueOf(number.shortValue());
+ } else if (argType == factory.charType) {
+ addition = String.valueOf((char) number.intValue());
+ } else if (argType == factory.intType) {
+ addition = String.valueOf(number.intValue());
+ } else if (argType == factory.longType) {
+ addition = String.valueOf(number.longValue());
+ } else if (argType == factory.floatType) {
+ addition = String.valueOf(number.floatValue());
+ } else if (argType == factory.doubleType) {
+ addition = String.valueOf(number.doubleValue());
+ }
+ } else if (arg.getTypeLattice().isNullType()) {
+ assert number.intValue() == 0;
+ addition = "null";
+ }
}
}
return addition;
}
+ private Number extractConstantNumber(Value arg) {
+ assert !arg.isPhi();
+ if (arg.definition.isConstNumber()) {
+ ConstNumber cst = arg.definition.asConstNumber();
+ if (cst.outType() == ValueType.LONG) {
+ return cst.getLongValue();
+ } else if (cst.outType() == ValueType.FLOAT) {
+ return cst.getFloatValue();
+ } else if (cst.outType() == ValueType.DOUBLE) {
+ return cst.getDoubleValue();
+ } else {
+ assert cst.outType() == ValueType.INT || cst.outType() == ValueType.OBJECT;
+ return cst.getIntValue();
+ }
+ } else if (arg.definition.isNumberConversion()) {
+ NumberConversion conversion = arg.definition.asNumberConversion();
+ assert conversion.inValues().size() == 1;
+ Number temp = extractConstantNumber(conversion.inValues().get(0));
+ if (temp == null) {
+ return null;
+ }
+ DexType conversionType = conversion.to.dexTypeFor(factory);
+ if (conversionType == factory.booleanType) {
+ return temp.intValue() != 0 ? 1 : 0;
+ } else if (conversionType == factory.byteType) {
+ return temp.byteValue();
+ } else if (conversionType == factory.shortType) {
+ return temp.shortValue();
+ } else if (conversionType == factory.charType) {
+ return temp.intValue();
+ } else if (conversionType == factory.intType) {
+ return temp.intValue();
+ } else if (conversionType == factory.longType) {
+ return temp.longValue();
+ } else if (conversionType == factory.floatType) {
+ return temp.floatValue();
+ } else if (conversionType == factory.doubleType) {
+ return temp.doubleValue();
+ }
+ }
+ return null;
+ }
+
private BuilderState findDominantState(
DominatorTree dominatorTree,
Map<Instruction, BuilderState> perInstrState,
@@ -690,11 +764,15 @@
numberOfBuildersWithNonTrivialStateChange++;
return false;
}
- for (DexType argType : invokedMethod.proto.parameters.values) {
- if (!canHandleArgumentType(argType)) {
- numberOfBuildersWithNonStringArg++;
- return false;
- }
+ assert invoke.inValues().size() == 2;
+ TypeLatticeElement argType = invoke.inValues().get(1).getTypeLattice();
+ if (!argType.isPrimitive() && !argType.isClassType() && !argType.isNullType()) {
+ numberOfBuildersWithUnsupportedArg++;
+ return false;
+ }
+ if (argType.isClassType()) {
+ DexType argClassType = argType.asClassTypeLatticeElement().getClassType();
+ return canHandleArgumentType(argClassType);
}
return true;
}
@@ -707,8 +785,6 @@
private boolean canHandleArgumentType(DexType argType) {
// TODO(b/113859361): passed to another builder should be an eligible case.
- // TODO(b/114002137): Improve arg extraction and type conversion.
- // For now, skip any append(arg) that receives non-string types.
return argType == factory.stringType || argType == factory.charSequenceType;
}
}
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 b21941a..c434aac 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
@@ -16,6 +16,7 @@
import com.android.tools.r8.ir.optimize.string.StringBuilderOptimizer.BuilderState;
import java.util.Map;
import java.util.function.Consumer;
+import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@@ -85,9 +86,13 @@
buildAndCheckIR(
"nonStringArgs",
checkOptimizerStates(appView, optimizer -> {
- // TODO(b/114002137): Improve arg extraction and type conversion.
- assertEquals(0, optimizer.analysis.builderStates.size());
- assertEquals(0, optimizer.analysis.simplifiedBuilders.size());
+ 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, "42", true);
+ }
+ assertEquals(1, optimizer.analysis.simplifiedBuilders.size());
}));
}
@@ -96,12 +101,17 @@
buildAndCheckIR(
"typeConversion",
checkOptimizerStates(appView, optimizer -> {
- // TODO(b/114002137): Improve arg extraction and type conversion.
- assertEquals(0, optimizer.analysis.builderStates.size());
- assertEquals(0, optimizer.analysis.simplifiedBuilders.size());
+ 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, "0.14 0 false null", true);
+ }
+ assertEquals(1, optimizer.analysis.simplifiedBuilders.size());
}));
}
+ @Ignore("TODO(b/113859361): passed to another builder should be an eligible case.")
@Test
public void testNestedBuilders_appendBuilderItself() throws Exception {
buildAndCheckIR(
@@ -139,7 +149,7 @@
buildAndCheckIR(
"simplePhi",
checkOptimizerStates(appView, optimizer -> {
- // TODO(b/114002137): Improve arg extraction and type conversion.
+ assertEquals(0, optimizer.analysis.builderStates.size());
assertEquals(0, optimizer.analysis.simplifiedBuilders.size());
}));
}
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 3f726f9..86cd835 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
@@ -69,6 +69,8 @@
int expectedStringCountInTrivialSequence,
int expectedStringCountInBuilderWithInitialValue,
int expectedStringCountInBuilderWithCapacity,
+ int expectedStringCountInNonStringArgs,
+ int expectedStringCountInTypeConversion,
int expectedStringCountInNestedBuilderAppendItself,
int expectedStringCountInNestedBuilderAppendResult)
throws Exception {
@@ -97,13 +99,13 @@
assertThat(method, isPresent());
count = Streams.stream(method.iterateInstructions(
i -> i.isConstString(JumboStringMode.ALLOW))).count();
- assertEquals(0, count);
+ assertEquals(expectedStringCountInNonStringArgs, count);
method = mainClass.uniqueMethodWithName("typeConversion");
assertThat(method, isPresent());
count = Streams.stream(method.iterateInstructions(
i -> i.isConstString(JumboStringMode.ALLOW))).count();
- assertEquals(0, count);
+ assertEquals(expectedStringCountInTypeConversion, count);
method = mainClass.uniqueMethodWithName("nestedBuilders_appendBuilderItself");
assertThat(method, isPresent());
@@ -121,7 +123,7 @@
assertThat(method, isPresent());
count = Streams.stream(method.iterateInstructions(
i -> i.isConstString(JumboStringMode.ALLOW))).count();
- assertEquals(5, count);
+ assertEquals(4, count);
method = mainClass.uniqueMethodWithName("phiAtInit");
assertThat(method, isPresent());
@@ -160,7 +162,7 @@
.addOptionsModification(this::configure)
.run(parameters.getRuntime(), MAIN)
.assertSuccessWithOutput(JAVA_OUTPUT);
- test(result, 3, 3, 2, 4, 4);
+ test(result, 3, 3, 0, 0, 0, 3, 3);
result =
testForD8()
@@ -172,7 +174,7 @@
.assertSuccessWithOutput(JAVA_OUTPUT);
// TODO(b/114002137): The lack of subtyping made the escape analysis to regard
// StringBuilder#toString as an alias-introducing instruction.
- test(result, 3, 3, 2, 4, 3);
+ test(result, 3, 3, 0, 0, 0, 3, 3);
}
@Test
@@ -189,7 +191,7 @@
.addOptionsModification(this::configure)
.run(parameters.getRuntime(), MAIN)
.assertSuccessWithOutput(JAVA_OUTPUT);
- test(result, 1, 1, 1, 4, 3);
+ test(result, 1, 1, 1, 1, 1, 3, 3);
}
// TODO(b/114002137): Once enabled, remove this test-specific setting.
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 da409fc..19c7129 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
@@ -25,9 +25,8 @@
@NeverInline
public static void builderWithCapacity() {
StringBuilder builder = new StringBuilder(3);
- // TODO(b/114002137): switch to use the integer.
- builder.append("4");
- builder.append("2");
+ builder.append(4);
+ builder.append(2);
System.out.println(builder.toString());
}
@@ -64,8 +63,7 @@
b1.append(",");
StringBuilder b2 = new StringBuilder();
b2.append("R");
- // TODO(b/114002137): switch to use the integer.
- b2.append("8");
+ b2.append(8);
b1.append(b2);
System.out.println(b1.toString());
}
@@ -77,8 +75,7 @@
b1.append(",");
StringBuilder b2 = new StringBuilder();
b2.append("R");
- // TODO(b/114002137): switch to use the integer.
- b2.append("8");
+ b2.append(8);
b1.append(b2.toString());
System.out.println(b1.toString());
}
@@ -95,8 +92,7 @@
} else {
builder.append("R");
}
- // TODO(b/114002137): switch to use the integer.
- builder.append("8");
+ builder.append(8);
System.out.println(builder.toString());
}
