Revert "Enable constructor inlining for final fields in tests"
This reverts commit e21e87abe6aaf23444f2a9fb65dad866bd3955f7.
Reason for revert: Test failures on Android 13+
Change-Id: Iefa843c9d1eefea51cdd85ed5f69b64d0bb98420
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/MethodProcessor.java b/src/main/java/com/android/tools/r8/ir/conversion/MethodProcessor.java
index e26ea21..9c8b286 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/MethodProcessor.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/MethodProcessor.java
@@ -8,10 +8,6 @@
public abstract class MethodProcessor {
- public boolean hasWaves() {
- return false;
- }
-
public boolean isD8MethodProcessor() {
return false;
}
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/PostMethodProcessor.java b/src/main/java/com/android/tools/r8/ir/conversion/PostMethodProcessor.java
index f625c8e..d31a884 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/PostMethodProcessor.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/PostMethodProcessor.java
@@ -85,11 +85,6 @@
}
@Override
- public boolean hasWaves() {
- return true;
- }
-
- @Override
public boolean isPostMethodProcessor() {
return true;
}
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/PrimaryMethodProcessor.java b/src/main/java/com/android/tools/r8/ir/conversion/PrimaryMethodProcessor.java
index 788bfcb..4e3f618 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/PrimaryMethodProcessor.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/PrimaryMethodProcessor.java
@@ -71,11 +71,6 @@
}
@Override
- public boolean hasWaves() {
- return true;
- }
-
- @Override
public boolean isPrimaryMethodProcessor() {
return true;
}
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 e811b72..f5c745d 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
@@ -18,7 +18,8 @@
import com.android.tools.r8.graph.AppView;
import com.android.tools.r8.graph.Code;
import com.android.tools.r8.graph.DefaultUseRegistryWithResult;
-import com.android.tools.r8.graph.DexClassAndField;
+import com.android.tools.r8.graph.DexEncodedField;
+import com.android.tools.r8.graph.DexField;
import com.android.tools.r8.graph.DexItemFactory;
import com.android.tools.r8.graph.DexItemFactory.BoxUnboxPrimitiveMethodRoundtrip;
import com.android.tools.r8.graph.DexMethod;
@@ -725,19 +726,20 @@
} else if (instruction.isInstancePut()) {
// Final fields may not be initialized outside of a constructor in the enclosing class.
InstancePut instancePut = instruction.asInstancePut();
- DexClassAndField target =
- instancePut.resolveField(appView, singleTarget).getResolutionPair();
+ DexField field = instancePut.getField();
+ DexEncodedField target = appView.appInfo().lookupInstanceTarget(field);
if (target == null) {
whyAreYouNotInliningReporter.reportUnsafeConstructorInliningDueToMissingFieldAssignment(
instancePut);
return false;
}
- if (target.getAccessFlags().isFinal()) {
+ if (target.isFinal()) {
+ // TODO(b/278964529): We should unset the final flag for good measure. Find a way to do
+ // this in a thread safe manner.
if (inlinerOptions.enableConstructorInliningWithFinalFields
- && options.canUseJavaLangVarHandleStoreStoreFence(appView)
- && methodProcessor.hasWaves()
- && target.isProgramField()) {
- actionBuilder.setShouldEnsureStoreStoreFence(target.asProgramField());
+ && options.canAssignFinalInstanceFieldOutsideConstructor()
+ && options.canUseJavaLangVarHandleStoreStoreFence(appView)) {
+ actionBuilder.setShouldEnsureStoreStoreBarrier();
} else {
whyAreYouNotInliningReporter.reportUnsafeConstructorInliningDueToFinalFieldAssignment(
instancePut);
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 af554ca..b58048e 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
@@ -21,7 +21,6 @@
import com.android.tools.r8.graph.DexType;
import com.android.tools.r8.graph.MethodResolutionResult.SingleResolutionResult;
import com.android.tools.r8.graph.NestMemberClassAttribute;
-import com.android.tools.r8.graph.ProgramField;
import com.android.tools.r8.graph.ProgramMethod;
import com.android.tools.r8.ir.analysis.ClassInitializationAnalysis;
import com.android.tools.r8.ir.analysis.proto.ProtoInliningReasonStrategy;
@@ -70,7 +69,6 @@
import com.android.tools.r8.utils.ListUtils;
import com.android.tools.r8.utils.Timing;
import com.android.tools.r8.utils.collections.LongLivedProgramMethodSetBuilder;
-import com.android.tools.r8.utils.collections.ProgramFieldSet;
import com.android.tools.r8.utils.collections.ProgramMethodMap;
import com.android.tools.r8.utils.collections.ProgramMethodSet;
import com.google.common.collect.ImmutableList;
@@ -86,7 +84,6 @@
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
-import java.util.concurrent.atomic.AtomicBoolean;
import java.util.function.Consumer;
import java.util.stream.Collectors;
@@ -103,10 +100,6 @@
private final MultiCallerInliner multiCallerInliner;
- // The set of final fields that have been assigned by a constructor that has been inlined in the
- // current wave.
- private final ProgramFieldSet inlinedFinalFieldsInWave = ProgramFieldSet.createConcurrent();
-
// The set of methods that have been single caller inlined in the current wave. These need to be
// pruned when the wave ends.
private final Map<DexProgramClass, ProgramMethodMap<ProgramMethod>>
@@ -116,8 +109,6 @@
private final AvailableApiExceptions availableApiExceptions;
- private final AtomicBoolean waveDoneScheduled = new AtomicBoolean();
-
public Inliner(AppView<AppInfoWithLiveness> appView) {
this(appView, null);
}
@@ -574,7 +565,7 @@
final Reason reason;
private boolean shouldEnsureStaticInitialization;
- private ProgramFieldSet shouldEnsureStoreStoreFenceCauses;
+ private boolean shouldEnsureStoreStoreBarrier;
private DexProgramClass downcastClass;
@@ -605,8 +596,8 @@
shouldEnsureStaticInitialization = true;
}
- void setShouldEnsureStoreStoreFenceCauses(ProgramFieldSet shouldEnsureStoreStoreFenceCauses) {
- this.shouldEnsureStoreStoreFenceCauses = shouldEnsureStoreStoreFenceCauses;
+ void setShouldEnsureStoreStoreBarrier() {
+ shouldEnsureStoreStoreBarrier = true;
}
boolean mustBeInlined() {
@@ -635,9 +626,9 @@
failingBlock -> synthesizeInitClass(code, failingBlock));
}
- // Insert a store-store fence at the end of the method.
- if (shouldEnsureStoreStoreFenceCauses != null) {
- synthesizeStoreStoreFence(appView, code);
+ // Insert a store-store barrier at the end of the method.
+ if (shouldEnsureStoreStoreBarrier) {
+ synthesizeStoreStoreBarrier(appView, code);
}
// Insert a null check if this is needed to preserve the implicit null check for the receiver.
@@ -834,7 +825,7 @@
}
}
- private void synthesizeStoreStoreFence(AppView<AppInfoWithLiveness> appView, IRCode code) {
+ private void synthesizeStoreStoreBarrier(AppView<AppInfoWithLiveness> appView, IRCode code) {
assert target.getDefinition().isInstanceInitializer();
BasicBlockIterator blockIterator = code.listIterator();
while (blockIterator.hasNext()) {
@@ -842,23 +833,10 @@
if (!block.isReturnBlock()) {
continue;
}
- InstructionListIterator instructionIterator;
- if (block.hasCatchHandlers()) {
- instructionIterator = block.listIterator();
- Instruction throwingInstruction =
- instructionIterator.nextUntil(Instruction::instructionTypeCanThrow);
- if (throwingInstruction != null) {
- instructionIterator =
- instructionIterator
- .splitCopyCatchHandlers(code, blockIterator, appView.options())
- .listIterator();
- } else {
- Instruction previousInstruction = instructionIterator.previous();
- assert previousInstruction.isReturn();
- }
- } else {
- instructionIterator = block.listIterator(block.getInstructions().size() - 1);
- }
+ // It is an invariant that return blocks do not have catch handlers.
+ assert !block.hasCatchHandlers();
+ InstructionListIterator instructionIterator =
+ block.listIterator(block.getInstructions().size() - 1);
DexMethod storeStoreFenceMethod =
appView.dexItemFactory().javaLangInvokeVarHandleMembers.storeStoreFence;
assert appView.definitionFor(storeStoreFenceMethod) != null;
@@ -889,7 +867,7 @@
private InvokeMethod invoke;
private Reason reason;
private boolean shouldEnsureStaticInitialization;
- private ProgramFieldSet shouldEnsureStoreStoreFenceCauses;
+ private boolean shouldEnsureStoreStoreBarrier;
private ProgramMethod target;
Builder setDowncastClass(DexProgramClass downcastClass) {
@@ -912,11 +890,8 @@
return this;
}
- Builder setShouldEnsureStoreStoreFence(ProgramField finalFieldCause) {
- if (shouldEnsureStoreStoreFenceCauses == null) {
- shouldEnsureStoreStoreFenceCauses = ProgramFieldSet.create();
- }
- shouldEnsureStoreStoreFenceCauses.add(finalFieldCause);
+ Builder setShouldEnsureStoreStoreBarrier() {
+ this.shouldEnsureStoreStoreBarrier = true;
return this;
}
@@ -933,8 +908,8 @@
if (shouldEnsureStaticInitialization) {
action.setShouldEnsureStaticInitialization();
}
- if (shouldEnsureStoreStoreFenceCauses != null) {
- action.setShouldEnsureStoreStoreFenceCauses(shouldEnsureStoreStoreFenceCauses);
+ if (shouldEnsureStoreStoreBarrier) {
+ action.setShouldEnsureStoreStoreBarrier();
}
return action;
}
@@ -1174,13 +1149,6 @@
iterator.inlineInvoke(
appView, code, inlinee, blockIterator, blocksToRemove, action.getDowncastClass());
- if (action.shouldEnsureStoreStoreFenceCauses != null) {
- assert !action.shouldEnsureStoreStoreFenceCauses.isEmpty();
- assert converter.isInWave();
- inlinedFinalFieldsInWave.addAll(action.shouldEnsureStoreStoreFenceCauses);
- scheduleWaveDone();
- }
-
if (methodProcessor.getCallSiteInformation().hasSingleCallSite(singleTarget, context)) {
feedback.markInlinedIntoSingleCallSite(singleTargetMethod);
appView.withArgumentPropagator(
@@ -1189,7 +1157,9 @@
singleTarget, context, methodProcessor));
if (!(methodProcessor instanceof OneTimeMethodProcessor)) {
assert converter.isInWave();
- scheduleWaveDone();
+ if (singleCallerInlinedMethodsInWave.isEmpty()) {
+ converter.addWaveDoneAction(this::onWaveDone);
+ }
singleCallerInlinedMethodsInWave
.computeIfAbsent(
singleTarget.getHolder(), ignoreKey(ProgramMethodMap::createConcurrent))
@@ -1369,7 +1339,6 @@
}
private void onWaveDone() {
- inlinedFinalFieldsInWave.forEach(field -> field.getAccessFlags().demoteFromFinal());
singleCallerInlinedMethodsInWave.forEach(
(clazz, singleCallerInlinedMethodsForClass) -> {
// Convert and remove virtual single caller inlined methods to abstract or throw null.
@@ -1415,15 +1384,7 @@
});
}
});
- inlinedFinalFieldsInWave.clear();
singleCallerInlinedMethodsInWave.clear();
- waveDoneScheduled.set(false);
- }
-
- private void scheduleWaveDone() {
- if (!waveDoneScheduled.getAndSet(true)) {
- converter.addWaveDoneAction(this::onWaveDone);
- }
}
public void onLastWaveDone(
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 db25078..6709446 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -2681,6 +2681,11 @@
return !appView.enableWholeProgramOptimizations();
}
+ public boolean canAssignFinalInstanceFieldOutsideConstructor() {
+ // ART does not check this property.
+ return isGeneratingDex();
+ }
+
/**
* Dex2Oat issues a warning for abstract methods on non-abstract classes, so we never allow this.
*
@@ -2882,7 +2887,7 @@
}
public boolean canUseJavaLangVarHandleStoreStoreFence(DexDefinitionSupplier definitions) {
- if (isGeneratingDex() && hasMinApi(AndroidApiLevel.T)) {
+ if (isGeneratingDex() && hasMinApi(AndroidApiLevel.P)) {
DexItemFactory factory = definitions.dexItemFactory();
return definitions.hasDefinitionFor(factory.javaLangInvokeVarHandleMembers.storeStoreFence);
}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/inliner/InlineConstructorWithFinalFieldsTest.java b/src/test/java/com/android/tools/r8/ir/optimize/inliner/InlineConstructorWithFinalFieldsTest.java
index 6e8e2f9..e5cca20 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/inliner/InlineConstructorWithFinalFieldsTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/inliner/InlineConstructorWithFinalFieldsTest.java
@@ -5,10 +5,9 @@
import static com.android.tools.r8.utils.codeinspector.CodeMatchers.invokesMethod;
import static com.android.tools.r8.utils.codeinspector.Matchers.isAbsentIf;
-import static com.android.tools.r8.utils.codeinspector.Matchers.isFinal;
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
-import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertFalse;
import com.android.tools.r8.AlwaysInline;
import com.android.tools.r8.TestBase;
@@ -16,9 +15,9 @@
import com.android.tools.r8.TestParametersCollection;
import com.android.tools.r8.ToolHelper;
import com.android.tools.r8.references.Reference;
+import com.android.tools.r8.utils.AndroidApiLevel;
import com.android.tools.r8.utils.MethodReferenceUtils;
import com.android.tools.r8.utils.codeinspector.ClassSubject;
-import com.android.tools.r8.utils.codeinspector.FieldSubject;
import com.android.tools.r8.utils.codeinspector.MethodSubject;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -46,6 +45,11 @@
parameters.isDexRuntime(),
testBuilder -> testBuilder.addLibraryFiles(ToolHelper.getMostRecentAndroidJar()))
.addKeepMainRule(Main.class)
+ .addOptionsModification(
+ options -> {
+ assertFalse(options.inlinerOptions().enableConstructorInliningWithFinalFields);
+ options.inlinerOptions().enableConstructorInliningWithFinalFields = true;
+ })
.enableAlwaysInliningAnnotations()
.setMinApi(parameters)
.compile()
@@ -54,24 +58,17 @@
ClassSubject mainClassSubject = inspector.clazz(Main.class);
assertThat(mainClassSubject, isPresent());
- FieldSubject xFieldSubject = mainClassSubject.uniqueFieldWithOriginalName("x");
- assertThat(xFieldSubject, isPresent());
-
- FieldSubject yFieldSubject = mainClassSubject.uniqueFieldWithOriginalName("y");
- assertThat(yFieldSubject, isPresent());
-
MethodSubject initMethodSubject = mainClassSubject.init("int", "int");
assertThat(
initMethodSubject,
- isAbsentIf(parameters.canUseJavaLangInvokeVarHandleStoreStoreFence()));
+ isAbsentIf(
+ parameters.isDexRuntime()
+ && parameters.getApiLevel().isGreaterThanOrEqualTo(AndroidApiLevel.P)));
MethodSubject mainMethodSubject = mainClassSubject.mainMethod();
assertThat(mainMethodSubject, isPresent());
-
if (initMethodSubject.isPresent()) {
assertThat(mainMethodSubject, invokesMethod(initMethodSubject));
- assertThat(xFieldSubject, isFinal());
- assertThat(yFieldSubject, isFinal());
} else {
assertThat(
mainMethodSubject,
@@ -81,8 +78,6 @@
invokesMethod(
Reference.methodFromDescriptor(
"Ljava/lang/invoke/VarHandle;", "storeStoreFence", "()V")));
- assertThat(xFieldSubject, not(isFinal()));
- assertThat(yFieldSubject, not(isFinal()));
}
})
.run(parameters.getRuntime(), Main.class, "20", "22")
diff --git a/src/test/testbase/java/com/android/tools/r8/TestCompilerBuilder.java b/src/test/testbase/java/com/android/tools/r8/TestCompilerBuilder.java
index f4f0034..2a9697a 100644
--- a/src/test/testbase/java/com/android/tools/r8/TestCompilerBuilder.java
+++ b/src/test/testbase/java/com/android/tools/r8/TestCompilerBuilder.java
@@ -67,7 +67,6 @@
options.testing.listIterationRewritingEnabled = true;
options.horizontalClassMergerOptions().enable();
options.horizontalClassMergerOptions().setEnableInterfaceMerging();
- options.inlinerOptions().enableConstructorInliningWithFinalFields = true;
options
.getCfCodeAnalysisOptions()
.setAllowUnreachableCfBlocks(false)
diff --git a/src/test/testbase/java/com/android/tools/r8/TestParameters.java b/src/test/testbase/java/com/android/tools/r8/TestParameters.java
index ab3077c..fc90c26 100644
--- a/src/test/testbase/java/com/android/tools/r8/TestParameters.java
+++ b/src/test/testbase/java/com/android/tools/r8/TestParameters.java
@@ -85,10 +85,6 @@
.isGreaterThanOrEqualTo(TestBase.apiLevelWithDefaultInterfaceMethodsSupport());
}
- public boolean canUseJavaLangInvokeVarHandleStoreStoreFence() {
- return isDexRuntime() && getApiLevel().isGreaterThanOrEqualTo(AndroidApiLevel.T);
- }
-
public boolean canUseNativeDexPC() {
assert isCfRuntime() || isDexRuntime();
return isDexRuntime() && getDexRuntimeVersion().isNewerThanOrEqual(DexVm.Version.V8_1_0);
