Rewrite invoke-static getter users to use staticized methods
ClassStaticizer was extended with support for getters but in
https://r8-review.googlesource.com/44841 but we failed to rewrite
users of those getters.
This was not spotted before since our local test was using
allowAccessModification, allowing us to member rebind the getter call
to just target the instance field.
Bug: 152800551
Change-Id: I2ae3ccc2eb6351ddf442490421670fa4729c4bb0
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/staticizer/StaticizingProcessor.java b/src/main/java/com/android/tools/r8/ir/optimize/staticizer/StaticizingProcessor.java
index 4a1924b..1d679ecc 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/staticizer/StaticizingProcessor.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/staticizer/StaticizingProcessor.java
@@ -13,6 +13,7 @@
import com.android.tools.r8.graph.DexEncodedMethod;
import com.android.tools.r8.graph.DexField;
import com.android.tools.r8.graph.DexItemFactory;
+import com.android.tools.r8.graph.DexMember;
import com.android.tools.r8.graph.DexMethod;
import com.android.tools.r8.graph.DexProgramClass;
import com.android.tools.r8.graph.DexType;
@@ -73,6 +74,7 @@
private final Map<DexEncodedMethod, CandidateInfo> hostClassInits = new IdentityHashMap<>();
private final Set<DexEncodedMethod> methodsToBeStaticized = Sets.newIdentityHashSet();
private final Map<DexField, CandidateInfo> singletonFields = new IdentityHashMap<>();
+ private final Map<DexMethod, CandidateInfo> singletonGetters = new IdentityHashMap<>();
private final Map<DexType, DexType> candidateToHostMapping = new IdentityHashMap<>();
StaticizingProcessor(
@@ -102,7 +104,7 @@
.add(collectOptimizationInfo(feedback)));
// Enqueue instance methods to be staticized (only remove references to 'this'). Intentionally
- // not collection optimization info for these methods, since they will be reprocessed again
+ // not collecting optimization info for these methods, since they will be reprocessed again
// below once staticized.
enqueueMethodsWithCodeOptimizations(
methodsToBeStaticized, optimizations -> optimizations.add(this::removeReferencesToThis));
@@ -211,19 +213,36 @@
for (DexEncodedMethod method : info.referencedFrom) {
IRCode code = method.buildIR(appView, appView.appInfo().originFor(method.holder()));
assert code != null;
- List<StaticGet> singletonFieldReads =
+ List<Instruction> singletonUsers =
Streams.stream(code.instructionIterator())
- .filter(Instruction::isStaticGet)
- .map(Instruction::asStaticGet)
- .filter(get -> get.getField() == info.singletonField.field)
+ .filter(
+ instruction -> {
+ if (instruction.isStaticGet()
+ && instruction.asStaticGet().getField() == info.singletonField.field) {
+ return true;
+ }
+ DexEncodedMethod getter = info.getter.get();
+ return getter != null
+ && instruction.isInvokeStatic()
+ && instruction.asInvokeStatic().getInvokedMethod() == getter.method;
+ })
.collect(Collectors.toList());
boolean fixableFieldReadsPerUsage = true;
- for (StaticGet read : singletonFieldReads) {
- Value dest = read.dest();
+ for (Instruction user : singletonUsers) {
+ if (user.outValue() == null) {
+ continue;
+ }
+ Value dest = user.outValue();
visited.clear();
trivialPhis.clear();
- boolean onlyHasTrivialPhis = testAndCollectPhisComposedOfSameFieldRead(
- visited, dest.uniquePhiUsers(), read.getField(), trivialPhis);
+ assert user.isInvokeStatic() || user.isStaticGet();
+ DexMember member =
+ user.isStaticGet()
+ ? user.asStaticGet().getField()
+ : user.asInvokeStatic().getInvokedMethod();
+ boolean onlyHasTrivialPhis =
+ testAndCollectPhisComposedOfSameMember(
+ visited, dest.uniquePhiUsers(), member, trivialPhis);
if (dest.hasPhiUsers() && !onlyHasTrivialPhis) {
fixableFieldReadsPerUsage = false;
break;
@@ -263,6 +282,10 @@
}
}
singletonFields.put(candidate.singletonField.field, candidate);
+ DexEncodedMethod getter = candidate.getter.get();
+ if (getter != null) {
+ singletonGetters.put(getter.method, candidate);
+ }
referencingExtraMethods.addAll(candidate.referencedFrom);
}
@@ -380,28 +403,38 @@
}
private void rewriteReferences(IRCode code, MethodProcessor methodProcessor) {
- // Process all singleton field reads and rewrite their users.
- List<StaticGet> singletonFieldReads =
+ // Fetch all instructions that reference singletons to avoid concurrent modifications to the
+ // instruction list that can arise from doing it directly in the iterator.
+ List<Instruction> singletonUsers =
Streams.stream(code.instructionIterator())
- .filter(Instruction::isStaticGet)
- .map(Instruction::asStaticGet)
- .filter(get -> singletonFields.containsKey(get.getField()))
+ .filter(
+ instruction ->
+ (instruction.isStaticGet()
+ && singletonFields.containsKey(
+ instruction.asFieldInstruction().getField()))
+ || (instruction.isInvokeStatic()
+ && singletonGetters.containsKey(
+ instruction.asInvokeStatic().getInvokedMethod())))
.collect(Collectors.toList());
-
- singletonFieldReads.forEach(
- read -> {
- DexField field = read.getField();
- CandidateInfo candidateInfo = singletonFields.get(field);
- assert candidateInfo != null;
- Value value = read.dest();
- if (value != null) {
- fixupStaticizedFieldReadUsers(code, value, field);
- }
- if (!candidateInfo.preserveRead.get()) {
- read.removeOrReplaceByDebugLocalRead(code);
- }
- });
-
+ for (Instruction singletonUser : singletonUsers) {
+ CandidateInfo candidateInfo;
+ DexMember member;
+ if (singletonUser.isStaticGet()) {
+ candidateInfo = singletonFields.get(singletonUser.asStaticGet().getField());
+ member = singletonUser.asStaticGet().getField();
+ } else {
+ assert singletonUser.isInvokeStatic();
+ candidateInfo = singletonGetters.get(singletonUser.asInvokeStatic().getInvokedMethod());
+ member = singletonUser.asInvokeStatic().getInvokedMethod();
+ }
+ Value value = singletonUser.outValue();
+ if (value != null) {
+ fixupStaticizedFieldUsers(code, value, member);
+ }
+ if (!candidateInfo.preserveRead.get()) {
+ singletonUser.removeOrReplaceByDebugLocalRead(code);
+ }
+ }
if (!candidateToHostMapping.isEmpty()) {
remapMovedCandidates(code);
}
@@ -468,7 +501,7 @@
// invoke-virtual { s1, ... } mtd1
// goto Exit
// b2:
- // s2 <- static-get singleton
+ // s2 <- invoke-static getter()
// ...
// invoke-virtual { s2, ... } mtd1
// goto Exit
@@ -482,7 +515,7 @@
// ...
// goto Exit
// b2:
- // s2 <- static-get singleton
+ // s2 <- invoke-static getter()
// ...
// goto Exit
// Exit:
@@ -493,8 +526,8 @@
// From staticizer's viewpoint, `sp` is trivial in the sense that it is composed of values that
// refer to the same singleton field. If so, we can safely relax the assertion; remove uses of
// field reads; remove quasi-trivial phis; and then remove original field reads.
- private boolean testAndCollectPhisComposedOfSameFieldRead(
- Set<Phi> visited, Set<Phi> phisToCheck, DexField field, Set<Phi> trivialPhis) {
+ private boolean testAndCollectPhisComposedOfSameMember(
+ Set<Phi> visited, Set<Phi> phisToCheck, DexMember dexMember, Set<Phi> trivialPhis) {
for (Phi phi : phisToCheck) {
if (!visited.add(phi)) {
continue;
@@ -505,16 +538,20 @@
if (v.isPhi()) {
chainedPhis.add(operand.asPhi());
} else {
- if (!v.definition.isStaticGet()) {
+ Instruction definition = v.definition;
+ if (!definition.isStaticGet() && !definition.isInvokeStatic()) {
return false;
}
- if (v.definition.asStaticGet().getField() != field) {
+ if (definition.isStaticGet() && definition.asStaticGet().getField() != dexMember) {
+ return false;
+ } else if (definition.isInvokeStatic()
+ && definition.asInvokeStatic().getInvokedMethod() != dexMember) {
return false;
}
}
}
if (!chainedPhis.isEmpty()) {
- if (!testAndCollectPhisComposedOfSameFieldRead(visited, chainedPhis, field, trivialPhis)) {
+ if (!testAndCollectPhisComposedOfSameMember(visited, chainedPhis, dexMember, trivialPhis)) {
return false;
}
}
@@ -525,13 +562,14 @@
// Fixup field read usages. Same as {@link #fixupStaticizedThisUsers} except this one determines
// quasi-trivial phis, based on the original field.
- private void fixupStaticizedFieldReadUsers(IRCode code, Value dest, DexField field) {
+ private void fixupStaticizedFieldUsers(IRCode code, Value dest, DexMember member) {
assert dest != null;
// During the examine phase, field reads with any phi users have been invalidated, hence zero.
// However, it may be not true if re-processing introduces phis after optimizing common suffix.
Set<Phi> trivialPhis = Sets.newIdentityHashSet();
- boolean onlyHasTrivialPhis = testAndCollectPhisComposedOfSameFieldRead(
- Sets.newIdentityHashSet(), dest.uniquePhiUsers(), field, trivialPhis);
+ boolean onlyHasTrivialPhis =
+ testAndCollectPhisComposedOfSameMember(
+ Sets.newIdentityHashSet(), dest.uniquePhiUsers(), member, trivialPhis);
assert !dest.hasPhiUsers() || onlyHasTrivialPhis;
assert trivialPhis.isEmpty() || onlyHasTrivialPhis;
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/staticizer/ClassStaticizerTest.java b/src/test/java/com/android/tools/r8/ir/optimize/staticizer/ClassStaticizerTest.java
index 5ab6053..b5ad06f 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/staticizer/ClassStaticizerTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/staticizer/ClassStaticizerTest.java
@@ -12,6 +12,7 @@
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
+import com.android.tools.r8.CompilationFailedException;
import com.android.tools.r8.NeverInline;
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
@@ -50,11 +51,14 @@
import com.android.tools.r8.ir.optimize.staticizer.trivial.TrivialTestClass;
import com.android.tools.r8.naming.MemberNaming.MethodSignature;
import com.android.tools.r8.utils.InternalOptions;
+import com.android.tools.r8.utils.StringUtils;
import com.android.tools.r8.utils.codeinspector.ClassSubject;
import com.android.tools.r8.utils.codeinspector.CodeInspector;
import com.google.common.collect.Lists;
import com.google.common.collect.Streams;
+import java.io.IOException;
import java.util.List;
+import java.util.concurrent.ExecutionException;
import java.util.stream.Collectors;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -64,6 +68,37 @@
public class ClassStaticizerTest extends TestBase {
private final TestParameters parameters;
+ private static final String EXPECTED =
+ StringUtils.lines(
+ "Simple::bar(Simple::foo())",
+ "Simple::bar(0)",
+ "SimpleWithPhi$Companion::bar(SimpleWithPhi$Companion::foo()) true",
+ "SimpleWithSideEffects::<clinit>()",
+ "SimpleWithSideEffects::bar(SimpleWithSideEffects::foo())",
+ "SimpleWithSideEffects::bar(1)",
+ "SimpleWithParams::bar(SimpleWithParams::foo())",
+ "SimpleWithParams::bar(2)",
+ "SimpleWithGetter::bar(SimpleWithGetter::foo())",
+ "SimpleWithGetter::bar(3)",
+ "Simple::bar(Simple::foo())",
+ "Simple::bar(4)",
+ "Simple::bar(Simple::foo())",
+ "Simple::bar(5)");
+
+ private static final Class<?> main = TrivialTestClass.class;
+ private static final Class<?>[] classes = {
+ NeverInline.class,
+ TrivialTestClass.class,
+ Simple.class,
+ SimpleWithGetter.class,
+ SimpleWithLazyInit.class,
+ SimpleWithParams.class,
+ SimpleWithPhi.class,
+ SimpleWithPhi.Companion.class,
+ SimpleWithSideEffects.class,
+ SimpleWithThrowingGetter.class
+ };
+
@Parameterized.Parameters(name = "{0}")
public static TestParametersCollection data() {
// TODO(b/112831361): support for class staticizer in CF backend.
@@ -75,21 +110,20 @@
}
@Test
+ public void testWithoutAccessModification()
+ throws ExecutionException, CompilationFailedException, IOException {
+ testForR8(parameters.getBackend())
+ .addProgramClasses(classes)
+ .addKeepMainRule(main)
+ .addKeepAttributes("InnerClasses", "EnclosingMethod")
+ .addOptionsModification(this::configure)
+ .setMinApi(parameters.getApiLevel())
+ .run(parameters.getRuntime(), main)
+ .assertSuccessWithOutput(EXPECTED);
+ }
+
+ @Test
public void testTrivial() throws Exception {
- Class<?> main = TrivialTestClass.class;
- Class<?>[] classes = {
- NeverInline.class,
- TrivialTestClass.class,
- Simple.class,
- SimpleWithGetter.class,
- SimpleWithLazyInit.class,
- SimpleWithParams.class,
- SimpleWithPhi.class,
- SimpleWithPhi.Companion.class,
- SimpleWithSideEffects.class,
- SimpleWithThrowingGetter.class
- };
- String javaOutput = runOnJava(main);
TestRunResult result =
testForR8(parameters.getBackend())
.addProgramClasses(classes)
@@ -101,7 +135,7 @@
.allowAccessModification()
.setMinApi(parameters.getApiLevel())
.run(parameters.getRuntime(), main)
- .assertSuccessWithOutput(javaOutput);
+ .assertSuccessWithOutput(EXPECTED);
CodeInspector inspector = result.inspector();
ClassSubject clazz = inspector.clazz(main);
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/staticizer/trivial/SimpleWithGetter.java b/src/test/java/com/android/tools/r8/ir/optimize/staticizer/trivial/SimpleWithGetter.java
index 55487ac..6bc91b4 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/staticizer/trivial/SimpleWithGetter.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/staticizer/trivial/SimpleWithGetter.java
@@ -15,11 +15,11 @@
@NeverInline
String foo() {
- return bar("Simple::foo()");
+ return bar("SimpleWithGetter::foo()");
}
@NeverInline
String bar(String other) {
- return "Simple::bar(" + other + ")";
+ return "SimpleWithGetter::bar(" + other + ")";
}
}
diff --git a/src/test/java/com/android/tools/r8/regress/b152800551/FailedStaticizingRegressionTest.java b/src/test/java/com/android/tools/r8/regress/b152800551/FailedStaticizingRegressionTest.java
index c764b70..7ec8ab0 100644
--- a/src/test/java/com/android/tools/r8/regress/b152800551/FailedStaticizingRegressionTest.java
+++ b/src/test/java/com/android/tools/r8/regress/b152800551/FailedStaticizingRegressionTest.java
@@ -3,7 +3,7 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8.regress.b152800551;
-import com.android.tools.r8.R8TestRunResult;
+import com.android.tools.r8.NeverInline;
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
@@ -12,6 +12,7 @@
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
+// This is a reproduction of b/152800551.
@RunWith(Parameterized.class)
public class FailedStaticizingRegressionTest extends TestBase {
@@ -40,20 +41,13 @@
@Test
public void testR8() throws Exception {
- R8TestRunResult runResult =
- testForR8(parameters.getBackend())
- .noMinification()
- .setMinApi(parameters.getApiLevel())
- .addInnerClasses(getClass())
- .addKeepMainRule(TestClass.class)
- .run(parameters.getRuntime(), TestClass.class);
-
- if (parameters.isDexRuntime()) {
- // TODO(b/152800551): Fails due to incorrect invoke-virtual to static method.
- runResult.assertFailureWithErrorThatThrows(IncompatibleClassChangeError.class);
- } else {
- runResult.assertSuccessWithOutput(EXPECTED);
- }
+ testForR8(parameters.getBackend())
+ .setMinApi(parameters.getApiLevel())
+ .addInnerClasses(getClass())
+ .addKeepMainRule(TestClass.class)
+ .enableInliningAnnotations()
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithOutput(EXPECTED);
}
static class S {
@@ -70,8 +64,8 @@
foo("b");
}
+ @NeverInline
public void foo(String s) {
- // Some duplication so this should not become inlined. Maybe use @NeverInline instead.
System.out.println("S::foo " + s + " 1");
System.out.println("S::foo " + s + " 2");
System.out.println("S::foo " + s + " 3");
