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"};