Revert "Synthesize InitClass instruction in inliner"
This reverts commit 72acc14c30da8342949f2103ce58e96647bcad14.
Revert "Update test expectations after inliner changes"
This reverts commit 6c18efff4e2a6bf71c8f7619a4dde7c54465dd68.
Change-Id: I297cdf190785fd6538543e0d3ae14481ca62ae5e
Reason for revert: Seemingly unrelated inliner error in YouTube 14.19
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/DefaultInliningOracle.java b/src/main/java/com/android/tools/r8/ir/optimize/DefaultInliningOracle.java
index 463bfce..f1c5dc9 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/DefaultInliningOracle.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/DefaultInliningOracle.java
@@ -128,6 +128,62 @@
return false;
}
+ private boolean canInlineStaticInvoke(
+ InvokeStatic invoke,
+ DexEncodedMethod method,
+ DexEncodedMethod target,
+ ClassInitializationAnalysis classInitializationAnalysis,
+ WhyAreYouNotInliningReporter whyAreYouNotInliningReporter) {
+ // Only proceed with inlining a static invoke if:
+ // - the holder for the target is a subtype of the holder for the method,
+ // - the target method always triggers class initialization of its holder before any other side
+ // effect (hence preserving class initialization semantics),
+ // - the current method has already triggered the holder for the target method to be
+ // initialized, or
+ // - there is no non-trivial class initializer.
+ DexType targetHolder = target.method.holder;
+ if (appView.appInfo().isSubtype(method.method.holder, targetHolder)) {
+ return true;
+ }
+ DexClass clazz = appView.definitionFor(targetHolder);
+ assert clazz != null;
+ if (target.getOptimizationInfo().triggersClassInitBeforeAnySideEffect()) {
+ return true;
+ }
+ if (!method.isStatic()) {
+ boolean targetIsGuaranteedToBeInitialized =
+ appView.withInitializedClassesInInstanceMethods(
+ analysis ->
+ analysis.isClassDefinitelyLoadedInInstanceMethodsOn(
+ target.method.holder, method.method.holder),
+ false);
+ if (targetIsGuaranteedToBeInitialized) {
+ return true;
+ }
+ }
+ if (classInitializationAnalysis.isClassDefinitelyLoadedBeforeInstruction(
+ target.method.holder, invoke)) {
+ return true;
+ }
+ // Check for class initializer side effects when loading this class, as inlining might remove
+ // the load operation.
+ //
+ // See https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-5.html#jvms-5.5.
+ //
+ // For simplicity, we are conservative and consider all interfaces, not only the ones with
+ // default methods.
+ if (!clazz.classInitializationMayHaveSideEffects(appView)) {
+ return true;
+ }
+
+ if (appView.rootSet().bypassClinitForInlining.contains(target.method)) {
+ return true;
+ }
+
+ whyAreYouNotInliningReporter.reportMustTriggerClassInitialization();
+ return false;
+ }
+
@Override
public boolean passesInliningConstraints(
InvokeMethod invoke,
@@ -335,71 +391,12 @@
Reason reason,
ClassInitializationAnalysis classInitializationAnalysis,
WhyAreYouNotInliningReporter whyAreYouNotInliningReporter) {
- InlineAction action = new InlineAction(singleTarget, invoke, reason);
- if (isTargetClassInitialized(invoke, method, singleTarget, classInitializationAnalysis)) {
- return action;
+ // Abort inlining attempt if we can not guarantee class for static target has been initialized.
+ if (!canInlineStaticInvoke(
+ invoke, method, singleTarget, classInitializationAnalysis, whyAreYouNotInliningReporter)) {
+ return null;
}
- if (appView.canUseInitClass()
- && appView.options().enableInliningOfInvokesWithClassInitializationSideEffects) {
- action.setShouldSynthesizeInitClass();
- return action;
- }
- whyAreYouNotInliningReporter.reportMustTriggerClassInitialization();
- return null;
- }
-
- private boolean isTargetClassInitialized(
- InvokeStatic invoke,
- DexEncodedMethod method,
- DexEncodedMethod target,
- ClassInitializationAnalysis classInitializationAnalysis) {
- // Only proceed with inlining a static invoke if:
- // - the holder for the target is a subtype of the holder for the method,
- // - the target method always triggers class initialization of its holder before any other side
- // effect (hence preserving class initialization semantics),
- // - the current method has already triggered the holder for the target method to be
- // initialized, or
- // - there is no non-trivial class initializer.
- DexType targetHolder = target.method.holder;
- if (appView.appInfo().isSubtype(method.method.holder, targetHolder)) {
- return true;
- }
- DexClass clazz = appView.definitionFor(targetHolder);
- assert clazz != null;
- if (target.getOptimizationInfo().triggersClassInitBeforeAnySideEffect()) {
- return true;
- }
- if (!method.isStatic()) {
- boolean targetIsGuaranteedToBeInitialized =
- appView.withInitializedClassesInInstanceMethods(
- analysis ->
- analysis.isClassDefinitelyLoadedInInstanceMethodsOn(
- target.method.holder, method.method.holder),
- false);
- if (targetIsGuaranteedToBeInitialized) {
- return true;
- }
- }
- if (classInitializationAnalysis.isClassDefinitelyLoadedBeforeInstruction(
- target.method.holder, invoke)) {
- return true;
- }
- // Check for class initializer side effects when loading this class, as inlining might remove
- // the load operation.
- //
- // See https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-5.html#jvms-5.5.
- //
- // For simplicity, we are conservative and consider all interfaces, not only the ones with
- // default methods.
- if (!clazz.classInitializationMayHaveSideEffects(appView)) {
- return true;
- }
-
- if (appView.rootSet().bypassClinitForInlining.contains(target.method)) {
- return true;
- }
-
- return false;
+ return new InlineAction(singleTarget, invoke, reason);
}
@Override
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/Inliner.java b/src/main/java/com/android/tools/r8/ir/optimize/Inliner.java
index 041d5a8..c9bf058 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/Inliner.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/Inliner.java
@@ -26,7 +26,6 @@
import com.android.tools.r8.ir.code.CatchHandlers.CatchHandler;
import com.android.tools.r8.ir.code.ConstClass;
import com.android.tools.r8.ir.code.IRCode;
-import com.android.tools.r8.ir.code.InitClass;
import com.android.tools.r8.ir.code.Instruction;
import com.android.tools.r8.ir.code.InstructionIterator;
import com.android.tools.r8.ir.code.InstructionListIterator;
@@ -574,7 +573,6 @@
public final Invoke invoke;
final Reason reason;
- private boolean shouldSynthesizeInitClass;
private boolean shouldSynthesizeNullCheckForReceiver;
InlineAction(DexEncodedMethod target, Invoke invoke, Reason reason) {
@@ -583,13 +581,7 @@
this.reason = reason;
}
- void setShouldSynthesizeInitClass() {
- assert !shouldSynthesizeNullCheckForReceiver;
- shouldSynthesizeInitClass = true;
- }
-
void setShouldSynthesizeNullCheckForReceiver() {
- assert !shouldSynthesizeInitClass;
shouldSynthesizeNullCheckForReceiver = true;
}
@@ -607,12 +599,6 @@
// Build the IR for a yet not processed method, and perform minimal IR processing.
IRCode code = inliningIRProvider.getInliningIR(invoke, target);
- // Insert a init class instruction if this is needed to preserve class initialization
- // semantics.
- if (shouldSynthesizeInitClass) {
- synthesizeInitClass(code);
- }
-
// Insert a null check if this is needed to preserve the implicit null check for the receiver.
// This is only needed if we do not also insert a monitor-enter instruction, since that will
// throw a NPE if the receiver is null.
@@ -751,21 +737,6 @@
return new InlineeWithReason(code, reason);
}
- private void synthesizeInitClass(IRCode code) {
- List<Value> arguments = code.collectArguments();
- BasicBlock entryBlock = code.entryBlock();
-
- // Insert a new block between the last argument instruction and the first actual instruction
- // of the method.
- BasicBlock initClassBlock =
- entryBlock.listIterator(code, arguments.size()).split(code, 0, null);
- assert !initClassBlock.hasCatchHandlers();
-
- InstructionListIterator iterator = initClassBlock.listIterator(code);
- iterator.setInsertionPosition(entryBlock.exit().getPosition());
- iterator.add(new InitClass(code.createValue(TypeLatticeElement.getInt()), target.holder()));
- }
-
private void synthesizeNullCheckForReceiver(AppView<?> appView, IRCode code) {
List<Value> arguments = code.collectArguments();
if (!arguments.isEmpty()) {
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 84d31f1..7ff5ff4 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -231,7 +231,6 @@
public boolean applyInliningToInlinee =
System.getProperty("com.android.tools.r8.applyInliningToInlinee") != null;
public int applyInliningToInlineeMaxDepth = 0;
- public boolean enableInliningOfInvokesWithClassInitializationSideEffects = true;
public boolean enableInliningOfInvokesWithNullableReceivers = true;
public boolean disableInliningOfLibraryMethodOverrides = true;
public boolean enableClassInlining = true;
diff --git a/src/test/java/com/android/tools/r8/classmerging/InliningAfterStaticClassMergerTest.java b/src/test/java/com/android/tools/r8/classmerging/InliningAfterStaticClassMergerTest.java
index 5fdd52b..23c2d55 100644
--- a/src/test/java/com/android/tools/r8/classmerging/InliningAfterStaticClassMergerTest.java
+++ b/src/test/java/com/android/tools/r8/classmerging/InliningAfterStaticClassMergerTest.java
@@ -102,18 +102,9 @@
FoundClassSubject clazz = classes.get(0);
assertEquals(StaticMergeCandidateA.class.getTypeName(), clazz.getOriginalName());
- if (parameters.isCfRuntime()) {
- // Check that StaticMergeCandidateB.m() has not been moved into StaticMergeCandidateA, because
- // that would disable inlining of it.
- assertEquals(1, clazz.allMethods().size());
- } else {
- // Check that StaticMergeCandidateB.m() has been inlined.
- assertEquals(0, clazz.allMethods().size());
-
- // Check that a static field has been synthesized in order to trigger class initialization.
- assertEquals(1, clazz.allStaticFields().size());
- assertEquals("$r8$clinit", clazz.allStaticFields().get(0).getOriginalName());
- }
+ // Check that StaticMergeCandidateB.m() has not been moved into StaticMergeCandidateA, because
+ // that would disable inlining of it.
+ assertEquals(1, clazz.allMethods().size());
// Check that the test class only has a main method.
ClassSubject classSubject = inspector.clazz(TestClass.class);
diff --git a/src/test/java/com/android/tools/r8/graph/initializedclasses/InitializedClassesInInstanceMethodsTest.java b/src/test/java/com/android/tools/r8/graph/initializedclasses/InitializedClassesInInstanceMethodsTest.java
index ee86dc8..43015bf 100644
--- a/src/test/java/com/android/tools/r8/graph/initializedclasses/InitializedClassesInInstanceMethodsTest.java
+++ b/src/test/java/com/android/tools/r8/graph/initializedclasses/InitializedClassesInInstanceMethodsTest.java
@@ -30,8 +30,7 @@
@Parameters(name = "{1}, enabled: {0}")
public static List<Object[]> data() {
- return buildParameters(
- BooleanUtils.values(), getTestParameters().withAllRuntimesAndApiLevels().build());
+ return buildParameters(BooleanUtils.values(), getTestParameters().withAllRuntimes().build());
}
public InitializedClassesInInstanceMethodsTest(
@@ -54,7 +53,7 @@
.allowAccessModification()
.enableNeverClassInliningAnnotations()
.enableInliningAnnotations()
- .setMinApi(parameters.getApiLevel())
+ .setMinApi(parameters.getRuntime())
.compile()
.inspect(this::verifyOutput)
.run(parameters.getRuntime(), TestClass.class)
@@ -72,15 +71,12 @@
assertThat(outerClassSubject.uniqueMethodWithName("exclamationMark"), not(isPresent()));
int numberOfExpectedAccessibilityBridges =
- enableInitializedClassesInInstanceMethodsAnalysis || parameters.isDexRuntime() ? 0 : 3;
+ enableInitializedClassesInInstanceMethodsAnalysis ? 0 : 3;
assertEquals(
numberOfExpectedAccessibilityBridges,
outerClassSubject
.allMethods(method -> method.getOriginalName().contains("access$"))
.size());
- assertEquals(
- parameters.isDexRuntime() && !enableInitializedClassesInInstanceMethodsAnalysis,
- outerClassSubject.uniqueFieldWithName("$r8$clinit").isPresent());
ClassSubject aClassSubject = inspector.clazz(Outer.A.class);
assertThat(aClassSubject, isPresent());
diff --git a/src/test/java/com/android/tools/r8/ir/analysis/sideeffect/SingletonClassInitializerPatternCannotBePostponedTest.java b/src/test/java/com/android/tools/r8/ir/analysis/sideeffect/SingletonClassInitializerPatternCannotBePostponedTest.java
index 4213323..d29753b 100644
--- a/src/test/java/com/android/tools/r8/ir/analysis/sideeffect/SingletonClassInitializerPatternCannotBePostponedTest.java
+++ b/src/test/java/com/android/tools/r8/ir/analysis/sideeffect/SingletonClassInitializerPatternCannotBePostponedTest.java
@@ -5,7 +5,6 @@
package com.android.tools.r8.ir.analysis.sideeffect;
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
-import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.MatcherAssert.assertThat;
import com.android.tools.r8.TestBase;
@@ -48,16 +47,9 @@
ClassSubject classSubject = inspector.clazz(A.class);
assertThat(classSubject, isPresent());
- if (parameters.isCfRuntime()) {
- // A.inlineable() cannot be inlined because it should trigger the class initialization of A,
- // which should trigger the class initialization of B, which will print "Hello".
- assertThat(classSubject.uniqueFieldWithName("$r8$clinit"), not(isPresent()));
- assertThat(classSubject.uniqueMethodWithName("inlineable"), isPresent());
- } else {
- // A static field A.$r8$clinit has been synthesized to allow inlining of A.inlineable().
- assertThat(classSubject.uniqueFieldWithName("$r8$clinit"), isPresent());
- assertThat(classSubject.uniqueMethodWithName("inlineable"), not(isPresent()));
- }
+ // A.inlineable() cannot be inlined because it should trigger the class initialization of A,
+ // which should trigger the class initialization of B, which will print "Hello".
+ assertThat(classSubject.uniqueMethodWithName("inlineable"), isPresent());
}
static class TestClass {
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/R8InliningTest.java b/src/test/java/com/android/tools/r8/ir/optimize/R8InliningTest.java
index ba0a4d1..a9385a6 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/R8InliningTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/R8InliningTest.java
@@ -15,6 +15,7 @@
import com.android.tools.r8.graph.DexEncodedMethod;
import com.android.tools.r8.graph.DexMethod;
import com.android.tools.r8.origin.Origin;
+import com.android.tools.r8.utils.AndroidApiLevel;
import com.android.tools.r8.utils.BooleanUtils;
import com.android.tools.r8.utils.FileUtils;
import com.android.tools.r8.utils.ZipUtils;
@@ -49,8 +50,7 @@
@Parameters(name = "{1}, allow access modification: {0}")
public static Collection<Object[]> data() {
- return buildParameters(
- BooleanUtils.values(), getTestParameters().withAllRuntimesAndApiLevels().build());
+ return buildParameters(BooleanUtils.values(), getTestParameters().withAllRuntimes().build());
}
private final boolean allowAccessModification;
@@ -105,7 +105,7 @@
commandBuilder.setProguardMapOutputPath(mapFile);
}
if (parameters.isDexRuntime()) {
- commandBuilder.setMinApiLevel(parameters.getApiLevel().getLevel());
+ commandBuilder.setMinApiLevel(AndroidApiLevel.M.getLevel());
}
if (allowAccessModification) {
commandBuilder.addProguardConfiguration(
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/inliner/InliningAfterClassInitializationTest.java b/src/test/java/com/android/tools/r8/ir/optimize/inliner/InliningAfterClassInitializationTest.java
index f9081c2..24ffad1 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/inliner/InliningAfterClassInitializationTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/inliner/InliningAfterClassInitializationTest.java
@@ -221,8 +221,6 @@
.addInnerClasses(InliningAfterClassInitializationTest.class)
.addLibraryFiles(ToolHelper.getAndroidJar(AndroidApiLevel.P))
.addKeepMainRule(mainClass)
- .addOptionsModification(
- options -> options.enableInliningOfInvokesWithClassInitializationSideEffects = false)
.enableConstantArgumentAnnotations()
.enableInliningAnnotations()
.setMinApi(parameters.getApiLevel())
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/inliner/InliningFromCurrentClassTest.java b/src/test/java/com/android/tools/r8/ir/optimize/inliner/InliningFromCurrentClassTest.java
index 27f3735..239b944 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/inliner/InliningFromCurrentClassTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/inliner/InliningFromCurrentClassTest.java
@@ -4,7 +4,6 @@
package com.android.tools.r8.ir.optimize.inliner;
-import static com.android.tools.r8.utils.codeinspector.CodeMatchers.accessesField;
import static com.android.tools.r8.utils.codeinspector.CodeMatchers.invokesMethod;
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
import static org.hamcrest.MatcherAssert.assertThat;
@@ -13,30 +12,14 @@
import com.android.tools.r8.NeverInline;
import com.android.tools.r8.NeverMerge;
import com.android.tools.r8.TestBase;
-import com.android.tools.r8.TestParameters;
-import com.android.tools.r8.TestParametersCollection;
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.android.tools.r8.utils.codeinspector.MethodSubject;
import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-@RunWith(Parameterized.class)
public class InliningFromCurrentClassTest extends TestBase {
- private final TestParameters parameters;
-
- @Parameterized.Parameters(name = "{0}")
- public static TestParametersCollection data() {
- return getTestParameters().withAllRuntimesAndApiLevels().build();
- }
-
- public InliningFromCurrentClassTest(TestParameters parameters) {
- this.parameters = parameters;
- }
-
@Test
public void test() throws Exception {
String expectedOutput =
@@ -46,18 +29,17 @@
"In A.inlineable1()",
"In B.inlineable2()",
"In C.<clinit>()",
- "In C.inlineableWithInitClass()");
+ "In C.notInlineable()");
testForJvm().addTestClasspath().run(TestClass.class).assertSuccessWithOutput(expectedOutput);
CodeInspector inspector =
- testForR8(parameters.getBackend())
+ testForR8(Backend.DEX)
.addInnerClasses(InliningFromCurrentClassTest.class)
.addKeepMainRule(TestClass.class)
.enableInliningAnnotations()
.enableMergeAnnotations()
- .setMinApi(parameters.getApiLevel())
- .run(parameters.getRuntime(), TestClass.class)
+ .run(TestClass.class)
.assertSuccessWithOutput(expectedOutput)
.inspector();
@@ -70,24 +52,18 @@
ClassSubject classC = inspector.clazz(C.class);
assertThat(classC, isPresent());
- MethodSubject testMethod = classB.uniqueMethodWithName("test");
- assertThat(testMethod, isPresent());
-
MethodSubject inlineable1Method = classA.uniqueMethodWithName("inlineable1");
assertThat(inlineable1Method, not(isPresent()));
MethodSubject inlineable2Method = classB.uniqueMethodWithName("inlineable2");
assertThat(inlineable2Method, not(isPresent()));
- MethodSubject inlineableWithInitClassMethod =
- classC.uniqueMethodWithName("inlineableWithInitClass");
- if (parameters.isCfRuntime()) {
- assertThat(inlineableWithInitClassMethod, isPresent());
- assertThat(testMethod, invokesMethod(inlineableWithInitClassMethod));
- } else {
- assertThat(inlineableWithInitClassMethod, not(isPresent()));
- assertThat(testMethod, accessesField(classC.uniqueFieldWithName("$r8$clinit")));
- }
+ MethodSubject notInlineableMethod = classC.uniqueMethodWithName("notInlineable");
+ assertThat(notInlineableMethod, isPresent());
+
+ MethodSubject testMethod = classB.uniqueMethodWithName("test");
+ assertThat(testMethod, isPresent());
+ assertThat(testMethod, invokesMethod(notInlineableMethod));
}
static class TestClass {
@@ -120,7 +96,7 @@
static void test() {
A.inlineable1();
B.inlineable2();
- C.inlineableWithInitClass();
+ C.notInlineable();
}
static void inlineable2() {
@@ -134,8 +110,8 @@
System.out.println("In C.<clinit>()");
}
- static void inlineableWithInitClass() {
- System.out.println("In C.inlineableWithInitClass()");
+ static void notInlineable() {
+ System.out.println("In C.notInlineable()");
}
}
}
diff --git a/src/test/java/com/android/tools/r8/utils/codeinspector/CodeMatchers.java b/src/test/java/com/android/tools/r8/utils/codeinspector/CodeMatchers.java
index 86a21c1..5dacc5a 100644
--- a/src/test/java/com/android/tools/r8/utils/codeinspector/CodeMatchers.java
+++ b/src/test/java/com/android/tools/r8/utils/codeinspector/CodeMatchers.java
@@ -4,7 +4,6 @@
package com.android.tools.r8.utils.codeinspector;
-import com.android.tools.r8.graph.DexField;
import com.android.tools.r8.graph.DexMethod;
import java.util.function.Predicate;
import org.hamcrest.Description;
@@ -13,35 +12,6 @@
public class CodeMatchers {
- public static Matcher<MethodSubject> accessesField(FieldSubject targetSubject) {
- if (!targetSubject.isPresent()) {
- throw new IllegalArgumentException();
- }
- DexField target = targetSubject.getField().field;
- return new TypeSafeMatcher<MethodSubject>() {
- @Override
- protected boolean matchesSafely(MethodSubject subject) {
- if (!subject.isPresent()) {
- return false;
- }
- if (!subject.getMethod().hasCode()) {
- return false;
- }
- return subject.streamInstructions().anyMatch(isFieldAccessWithTarget(target));
- }
-
- @Override
- public void describeTo(Description description) {
- description.appendText("accesses field `" + target.toSourceString() + "`");
- }
-
- @Override
- public void describeMismatchSafely(final MethodSubject subject, Description description) {
- description.appendText("method did not");
- }
- };
- }
-
public static Matcher<MethodSubject> invokesMethod(MethodSubject targetSubject) {
if (!targetSubject.isPresent()) {
throw new IllegalArgumentException();
@@ -74,8 +44,4 @@
public static Predicate<InstructionSubject> isInvokeWithTarget(DexMethod target) {
return instruction -> instruction.isInvoke() && instruction.getMethod() == target;
}
-
- public static Predicate<InstructionSubject> isFieldAccessWithTarget(DexField target) {
- return instruction -> instruction.isFieldAccess() && instruction.getField() == target;
- }
}
