Enable and remove InternalOptions' experimentalNewFilledArraySupport
Plus fix 2 bugs with it and fixup one test.
Bugs:
* Not checking that array is the destination of aput-object users
* Not checking whether value type is an array for isSubtype()
Bug: 246971330
Change-Id: I4a75e439eb46ad9a295fa3ec6821aa132a80a88d
diff --git a/src/main/java/com/android/tools/r8/ir/analysis/proto/GeneratedMessageLiteShrinker.java b/src/main/java/com/android/tools/r8/ir/analysis/proto/GeneratedMessageLiteShrinker.java
index b010a58..8a6298c 100644
--- a/src/main/java/com/android/tools/r8/ir/analysis/proto/GeneratedMessageLiteShrinker.java
+++ b/src/main/java/com/android/tools/r8/ir/analysis/proto/GeneratedMessageLiteShrinker.java
@@ -320,8 +320,7 @@
Value newObjectsValue = code.createValue(objectArrayType);
// Populate the `objects` array.
- if (appView.options().rewriteArrayOptions().experimentalNewFilledArraySupport
- && appView.options().rewriteArrayOptions().canUseFilledNewArrayOfObjects()) {
+ if (appView.options().rewriteArrayOptions().canUseFilledNewArrayOfObjects()) {
List<Value> arrayValues = new ArrayList<>(objects.size());
for (int i = 0; i < objects.size(); i++) {
Instruction materializingInstruction = objects.get(i).buildIR(appView, code);
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java b/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
index cade4ce..5406e16 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
@@ -2134,6 +2134,7 @@
private Value[] computeArrayValues(LinearFlowInstructionListIterator it, int size) {
NewArrayEmpty newArrayEmpty = it.next().asNewArrayEmpty();
assert newArrayEmpty != null;
+ Value arrayValue = newArrayEmpty.outValue();
// aput-object allows any object for arrays of interfaces, but new-filled-array fails to verify
// if types require a cast.
@@ -2165,7 +2166,7 @@
ArrayPut arrayPut = instruction.asArrayPut();
// If the initialization sequence is broken by another use we cannot use a fill-array-data
// instruction.
- if (arrayPut == null) {
+ if (arrayPut == null || arrayPut.array() != arrayValue) {
return null;
}
if (!arrayPut.index().isConstNumber()) {
@@ -2185,6 +2186,9 @@
if (!elementType.equals(valueDexType)) {
return null;
}
+ } else if (valueDexType.isArrayType()) {
+ // isSubtype asserts for this case.
+ return null;
} else {
// TODO(b/246971330): When in d8 mode, we might still be able to see if this is true for
// library types (which this helper does not do).
@@ -2246,18 +2250,11 @@
if (values == null) {
continue;
}
- if (!rewriteOptions.experimentalNewFilledArraySupport
- && !newArrayEmpty.type.isPrimitiveArrayType()
- && newArrayEmpty.type != dexItemFactory.stringArrayType) {
- continue;
- }
// filled-new-array is implemented only for int[] and Object[].
// For int[], using filled-new-array is usually smaller than filled-array-data.
// filled-new-array supports up to 5 registers before it's filled-new-array/range.
if (!newArrayEmpty.type.isPrimitiveArrayType()
- || (rewriteOptions.experimentalNewFilledArraySupport
- && newArrayEmpty.type == dexItemFactory.intArrayType
- && size <= 5)) {
+ || (newArrayEmpty.type == dexItemFactory.intArrayType && size <= 5)) {
// Don't replace with filled-new-array if it requires more than 200 consecutive registers.
if (size > rewriteOptions.maxRangeInputs) {
continue;
@@ -2309,6 +2306,7 @@
// inserted a fill array data instruction instead.
if (!storesToRemoveForArray.isEmpty()) {
Set<BasicBlock> visitedBlocks = Sets.newIdentityHashSet();
+ int numInstructionsInserted = 0;
do {
visitedBlocks.add(block);
it = block.listIterator(code);
@@ -2330,6 +2328,7 @@
// last removed put at which point we are now adding the construction.
construction.setPosition(instruction.getPosition());
it.add(construction);
+ numInstructionsInserted += 1;
}
}
}
@@ -2338,6 +2337,7 @@
BasicBlock nextBlock = block.exit().isGoto() ? block.exit().asGoto().getTarget() : null;
block = nextBlock != null && !visitedBlocks.contains(nextBlock) ? nextBlock : null;
} while (block != null);
+ assert numInstructionsInserted == instructionToInsertForArray.size();
}
}
assert code.isConsistentSSA(appView);
diff --git a/src/main/java/com/android/tools/r8/utils/InternalOptions.java b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
index acf5ac1..a0c146a 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -1394,8 +1394,6 @@
// Arbitrary limit of number of inputs to fill-array-data.
public int maxFillArrayDataInputs = 8 * 1024;
- // TODO(b/246971330): Remove when feature is complete.
- public boolean experimentalNewFilledArraySupport = false;
// Dalvik x86-atom backend had a bug that made it crash on filled-new-array instructions for
// arrays of objects. This is unfortunate, since this never hits arm devices, but we have
// to disallow filled-new-array of objects for dalvik until kitkat. The buggy code was
diff --git a/src/test/java/com/android/tools/r8/compatproguard/GetMembersTest.java b/src/test/java/com/android/tools/r8/compatproguard/GetMembersTest.java
index 812eb63..80e3c1e 100644
--- a/src/test/java/com/android/tools/r8/compatproguard/GetMembersTest.java
+++ b/src/test/java/com/android/tools/r8/compatproguard/GetMembersTest.java
@@ -6,12 +6,11 @@
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertTrue;
-import com.android.tools.r8.dex.code.DexAputObject;
-import com.android.tools.r8.dex.code.DexConst4;
import com.android.tools.r8.dex.code.DexConstClass;
import com.android.tools.r8.dex.code.DexConstString;
+import com.android.tools.r8.dex.code.DexFilledNewArray;
import com.android.tools.r8.dex.code.DexInvokeVirtual;
-import com.android.tools.r8.dex.code.DexNewArray;
+import com.android.tools.r8.dex.code.DexMoveResultObject;
import com.android.tools.r8.dex.code.DexReturnVoid;
import com.android.tools.r8.graph.DexCode;
import com.android.tools.r8.smali.SmaliBuilder;
@@ -93,16 +92,14 @@
assertTrue(method.isPresent());
DexCode code = method.getMethod().getCode().asDexCode();
- assertTrue(code.instructions[0] instanceof DexConst4);
- assertTrue(code.instructions[1] instanceof DexNewArray);
- assertTrue(code.instructions[2] instanceof DexConst4);
+ assertTrue(code.instructions[0] instanceof DexConstClass);
+ assertTrue(code.instructions[1] instanceof DexFilledNewArray);
+ assertTrue(code.instructions[2] instanceof DexMoveResultObject);
assertTrue(code.instructions[3] instanceof DexConstClass);
- assertTrue(code.instructions[4] instanceof DexAputObject);
- assertTrue(code.instructions[5] instanceof DexConstClass);
- assertTrue(code.instructions[6] instanceof DexConstString);
- assertNotEquals("foo", code.instructions[6].asConstString().getString().toString());
- assertTrue(code.instructions[7] instanceof DexInvokeVirtual);
- assertTrue(code.instructions[8] instanceof DexReturnVoid);
+ assertTrue(code.instructions[4] instanceof DexConstString);
+ assertNotEquals("foo", code.instructions[4].asConstString().getString().toString());
+ assertTrue(code.instructions[5] instanceof DexInvokeVirtual);
+ assertTrue(code.instructions[6] instanceof DexReturnVoid);
}
}
diff --git a/src/test/java/com/android/tools/r8/naming/IdentifierNameStringMarkerTest.java b/src/test/java/com/android/tools/r8/naming/IdentifierNameStringMarkerTest.java
index fd3ba45..2ccc111 100644
--- a/src/test/java/com/android/tools/r8/naming/IdentifierNameStringMarkerTest.java
+++ b/src/test/java/com/android/tools/r8/naming/IdentifierNameStringMarkerTest.java
@@ -639,16 +639,7 @@
"-keep class " + CLASS_NAME,
"-keep class R { *; }");
CodeInspector inspector =
- compileWithR8(
- builder,
- testBuilder ->
- testBuilder
- .addKeepRules(pgConfigs)
- .addOptionsModification(
- options ->
- options.rewriteArrayOptions().experimentalNewFilledArraySupport =
- true))
- .inspector();
+ compileWithR8(builder, testBuilder -> testBuilder.addKeepRules(pgConfigs)).inspector();
ClassSubject clazz = inspector.clazz(CLASS_NAME);
assertTrue(clazz.isPresent());
@@ -708,16 +699,7 @@
"-keep class " + CLASS_NAME,
"-keep,allowobfuscation class R { *; }");
CodeInspector inspector =
- compileWithR8(
- builder,
- testBuilder ->
- testBuilder
- .addKeepRules(pgConfigs)
- .addOptionsModification(
- (options) ->
- options.rewriteArrayOptions().experimentalNewFilledArraySupport =
- true))
- .inspector();
+ compileWithR8(builder, testBuilder -> testBuilder.addKeepRules(pgConfigs)).inspector();
ClassSubject clazz = inspector.clazz(CLASS_NAME);
assertTrue(clazz.isPresent());
diff --git a/src/test/java/com/android/tools/r8/rewrite/arrays/ArrayWithDataLengthRewriteTest.java b/src/test/java/com/android/tools/r8/rewrite/arrays/ArrayWithDataLengthRewriteTest.java
index 73e39e1..513dc73 100644
--- a/src/test/java/com/android/tools/r8/rewrite/arrays/ArrayWithDataLengthRewriteTest.java
+++ b/src/test/java/com/android/tools/r8/rewrite/arrays/ArrayWithDataLengthRewriteTest.java
@@ -43,8 +43,6 @@
.setMinApi(parameters.getApiLevel())
.setMode(CompilationMode.RELEASE)
.addProgramClasses(Main.class)
- .addOptionsModification(
- opt -> opt.rewriteArrayOptions().experimentalNewFilledArraySupport = true)
.addOptionsModification(opt -> opt.testing.irModifier = this::transformArray)
.run(parameters.getRuntime(), Main.class)
.assertSuccessWithOutputLines(expectedOutput)
@@ -56,8 +54,6 @@
testForR8(parameters.getBackend())
.setMinApi(parameters.getRuntime())
.addProgramClasses(Main.class)
- .addOptionsModification(
- opt -> opt.rewriteArrayOptions().experimentalNewFilledArraySupport = true)
.addOptionsModification(opt -> opt.testing.irModifier = this::transformArray)
.addKeepMainRule(Main.class)
.enableInliningAnnotations()
diff --git a/src/test/java/com/android/tools/r8/rewrite/arrays/SimplifyArrayConstructionTest.java b/src/test/java/com/android/tools/r8/rewrite/arrays/SimplifyArrayConstructionTest.java
index 4fe27d8..6c6e076 100644
--- a/src/test/java/com/android/tools/r8/rewrite/arrays/SimplifyArrayConstructionTest.java
+++ b/src/test/java/com/android/tools/r8/rewrite/arrays/SimplifyArrayConstructionTest.java
@@ -97,6 +97,7 @@
"[0, 1, 2]",
"[1, 2, 3]",
"[1, 2, 3, 4, 5, 6]",
+ "[0]",
};
private static final byte[] TRANSFORMED_MAIN = transformedMain();
@@ -109,9 +110,6 @@
d8TestBuilder ->
d8TestBuilder
.setMinApi(parameters.getApiLevel())
- .addOptionsModification(
- options ->
- options.rewriteArrayOptions().experimentalNewFilledArraySupport = true)
.setMode(compilationMode))
.addProgramClassFileData(TRANSFORMED_MAIN)
.run(parameters.getRuntime(), Main.class)
@@ -124,12 +122,10 @@
testForR8(parameters.getBackend())
.setMinApi(parameters.getApiLevel())
.addOptionsModification(
- options -> {
- options.rewriteArrayOptions().experimentalNewFilledArraySupport = true;
- options
- .getOpenClosedInterfacesOptions()
- .suppressSingleOpenInterface(Reference.classFromClass(Serializable.class));
- })
+ options ->
+ options
+ .getOpenClosedInterfacesOptions()
+ .suppressSingleOpenInterface(Reference.classFromClass(Serializable.class)))
.setMode(compilationMode)
.addProgramClassFileData(TRANSFORMED_MAIN)
.addKeepMainRule(Main.class)
@@ -296,6 +292,7 @@
catchHandlerThrowing();
catchHandlerNonThrowingFilledNewArray();
catchHandlerNonThrowingFillArrayData();
+ arrayIntoAnotherArray();
}
@NeverInline
@@ -432,6 +429,16 @@
}
@NeverInline
+ private static void arrayIntoAnotherArray() {
+ // Tests that our computeValues() method does not assume all array-put-object users have
+ // the array as the target.
+ int[] intArr = new int[1];
+ Object[] objArr = new Object[2];
+ objArr[0] = intArr;
+ System.out.println(Arrays.toString((int[]) objArr[0]));
+ }
+
+ @NeverInline
private static void phiFilledNewArray() {
// The presence of ? should not affect use of filled-new-array.
String[] phiArray = {"a", System.nanoTime() > 0 ? "b" : "c"};
