Account for classes with kept classes in default field value analysis
Fixes: b/379034741
Change-Id: I230714181f351334d97ee0f60cd0252dcb5710e1
diff --git a/src/main/java/com/android/tools/r8/optimize/argumentpropagation/ArgumentPropagatorOptimizationInfoPropagator.java b/src/main/java/com/android/tools/r8/optimize/argumentpropagation/ArgumentPropagatorOptimizationInfoPropagator.java
index 800b277..c403ea1 100644
--- a/src/main/java/com/android/tools/r8/optimize/argumentpropagation/ArgumentPropagatorOptimizationInfoPropagator.java
+++ b/src/main/java/com/android/tools/r8/optimize/argumentpropagation/ArgumentPropagatorOptimizationInfoPropagator.java
@@ -84,6 +84,7 @@
converter,
fieldStates,
methodStates,
+ immediateSubtypingInfo,
inFlowComparator)
.run(executorService);
timing.end();
diff --git a/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/DefaultFieldValueJoiner.java b/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/DefaultFieldValueJoiner.java
index 28d0ead..4c9489a 100644
--- a/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/DefaultFieldValueJoiner.java
+++ b/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/DefaultFieldValueJoiner.java
@@ -11,6 +11,7 @@
import com.android.tools.r8.graph.DefaultUseRegistryWithResult;
import com.android.tools.r8.graph.DexProgramClass;
import com.android.tools.r8.graph.DexType;
+import com.android.tools.r8.graph.ImmediateProgramSubtypingInfo;
import com.android.tools.r8.graph.ProgramField;
import com.android.tools.r8.graph.ProgramMethod;
import com.android.tools.r8.ir.analysis.type.DynamicType;
@@ -36,6 +37,8 @@
import com.android.tools.r8.utils.ThreadUtils;
import com.android.tools.r8.utils.Timing;
import com.android.tools.r8.utils.collections.ProgramFieldSet;
+import it.unimi.dsi.fastutil.objects.Reference2BooleanMap;
+import it.unimi.dsi.fastutil.objects.Reference2BooleanOpenHashMap;
import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Collection;
@@ -57,17 +60,20 @@
private final Set<DexProgramClass> classesWithSingleCallerInlinedInstanceInitializers;
private final FieldStateCollection fieldStates;
private final List<FlowGraph> flowGraphs;
+ private final ImmediateProgramSubtypingInfo immediateSubtypingInfo;
public DefaultFieldValueJoiner(
AppView<AppInfoWithLiveness> appView,
Set<DexProgramClass> classesWithSingleCallerInlinedInstanceInitializers,
FieldStateCollection fieldStates,
- List<FlowGraph> flowGraphs) {
+ List<FlowGraph> flowGraphs,
+ ImmediateProgramSubtypingInfo immediateSubtypingInfo) {
this.appView = appView;
this.classesWithSingleCallerInlinedInstanceInitializers =
classesWithSingleCallerInlinedInstanceInitializers;
this.fieldStates = fieldStates;
this.flowGraphs = flowGraphs;
+ this.immediateSubtypingInfo = immediateSubtypingInfo;
}
public Map<FlowGraph, Deque<FlowGraphNode>> joinDefaultFieldValuesForFieldsWithReadBeforeWrite(
@@ -79,12 +85,35 @@
return Collections.emptyMap();
}
+ // Classes that are kept or have a kept subclass can be instantiated in a way that does not use
+ // any instance initializers. For all fields on such classes, we therefore include the default
+ // field value.
+ Map<DexProgramClass, Boolean> classesWithKeptSubclasses =
+ computeClassesWithKeptSubclasses(fieldsOfInterest);
+ ProgramFieldSet fieldsWithLiveDefaultValue = ProgramFieldSet.createConcurrent();
+ MapUtils.removeIf(
+ fieldsOfInterest,
+ (clazz, fields) -> {
+ Boolean isKeptOrHasKeptSubclass = classesWithKeptSubclasses.get(clazz);
+ assert isKeptOrHasKeptSubclass != null;
+ if (isKeptOrHasKeptSubclass) {
+ fields.removeIf(
+ field -> {
+ if (field.getDefinition().isInstance()) {
+ fieldsWithLiveDefaultValue.add(field);
+ return true;
+ }
+ return false;
+ });
+ }
+ return fields.isEmpty();
+ });
+
// If constructor inlining is disabled, then we focus on whether each instance initializer
// definitely assigns the given field before it is read. We do the same for final and static
// fields.
Map<DexType, ProgramFieldSet> fieldsNotSubjectToInitializerAnalysis =
removeFieldsNotSubjectToInitializerAnalysis(fieldsOfInterest);
- ProgramFieldSet fieldsWithLiveDefaultValue = ProgramFieldSet.createConcurrent();
analyzeInitializers(
fieldsOfInterest,
field -> {
@@ -108,7 +137,46 @@
return updateFlowGraphs(fieldsWithLiveDefaultValue, executorService);
}
- protected Map<DexProgramClass, List<ProgramField>> getFieldsOfInterest() {
+ /**
+ * For each of the classes in the key set of the given {@param fieldsOfInterest}, this computes
+ * whether the class is kept or has a kept subclass.
+ */
+ private Map<DexProgramClass, Boolean> computeClassesWithKeptSubclasses(
+ Map<DexProgramClass, List<ProgramField>> fieldsOfInterest) {
+ Reference2BooleanMap<DexProgramClass> classesWithKeptSubclasses =
+ new Reference2BooleanOpenHashMap<>();
+ for (DexProgramClass clazz : fieldsOfInterest.keySet()) {
+ computeClassHasKeptSubclass(clazz, classesWithKeptSubclasses);
+ }
+ return classesWithKeptSubclasses;
+ }
+
+ private boolean computeClassHasKeptSubclass(
+ DexProgramClass clazz, Reference2BooleanMap<DexProgramClass> classesWithKeptSubclasses) {
+ if (classesWithKeptSubclasses.containsKey(clazz)) {
+ return classesWithKeptSubclasses.getBoolean(clazz);
+ }
+ if (isKeptDirectly(clazz)) {
+ classesWithKeptSubclasses.put(clazz, true);
+ return true;
+ }
+ for (DexProgramClass subclass : immediateSubtypingInfo.getSubclasses(clazz)) {
+ if (computeClassHasKeptSubclass(subclass, classesWithKeptSubclasses)) {
+ classesWithKeptSubclasses.put(clazz, true);
+ return true;
+ }
+ }
+ assert !classesWithKeptSubclasses.containsKey(clazz)
+ || !classesWithKeptSubclasses.getBoolean(clazz);
+ classesWithKeptSubclasses.put(clazz, false);
+ return false;
+ }
+
+ private boolean isKeptDirectly(DexProgramClass clazz) {
+ return appView.getKeepInfo(clazz).isPinned(appView.options());
+ }
+
+ private Map<DexProgramClass, List<ProgramField>> getFieldsOfInterest() {
Map<DexProgramClass, List<ProgramField>> fieldsOfInterest = new IdentityHashMap<>();
for (DexProgramClass clazz : appView.appInfo().classes()) {
clazz.forEachProgramField(
@@ -262,12 +330,6 @@
(holderType, fields) -> {
assert !fields.isEmpty();
DexProgramClass holder = fields.iterator().next().getHolder();
- // If the class is kept it could be instantiated directly, in which case all default field
- // values could be live.
- if (appView.getKeepInfo(holder).isPinned(appView.options())) {
- fields.forEach(liveDefaultValueConsumer);
- return true;
- }
if (holder.isFinal() || !appView.appInfo().isInstantiatedIndirectly(holder)) {
// When the class is not explicitly marked final, the class could in principle have
// injected subclasses if it is pinned. However, none of the fields are pinned, so we
diff --git a/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/InFlowPropagator.java b/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/InFlowPropagator.java
index 752e59d..ee8e703 100644
--- a/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/InFlowPropagator.java
+++ b/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/InFlowPropagator.java
@@ -9,6 +9,7 @@
import com.android.tools.r8.graph.AppView;
import com.android.tools.r8.graph.DexProgramClass;
import com.android.tools.r8.graph.DexType;
+import com.android.tools.r8.graph.ImmediateProgramSubtypingInfo;
import com.android.tools.r8.graph.ProgramField;
import com.android.tools.r8.graph.ProgramMethod;
import com.android.tools.r8.ir.analysis.type.DynamicType;
@@ -50,6 +51,7 @@
final IRConverter converter;
protected final FieldStateCollection fieldStates;
final MethodStateCollectionByReference methodStates;
+ final ImmediateProgramSubtypingInfo immediateSubtypingInfo;
final InFlowComparator inFlowComparator;
public InFlowPropagator(
@@ -58,6 +60,7 @@
IRConverter converter,
FieldStateCollection fieldStates,
MethodStateCollectionByReference methodStates,
+ ImmediateProgramSubtypingInfo immediateSubtypingInfo,
InFlowComparator inFlowComparator) {
this.appView = appView;
this.classesWithSingleCallerInlinedInstanceInitializers =
@@ -65,6 +68,7 @@
this.converter = converter;
this.fieldStates = fieldStates;
this.methodStates = methodStates;
+ this.immediateSubtypingInfo = immediateSubtypingInfo;
this.inFlowComparator = inFlowComparator;
}
@@ -124,7 +128,11 @@
protected DefaultFieldValueJoiner createDefaultFieldValueJoiner(List<FlowGraph> flowGraphs) {
return new DefaultFieldValueJoiner(
- appView, classesWithSingleCallerInlinedInstanceInitializers, fieldStates, flowGraphs);
+ appView,
+ classesWithSingleCallerInlinedInstanceInitializers,
+ fieldStates,
+ flowGraphs,
+ immediateSubtypingInfo);
}
private void processFlowGraphs(List<FlowGraph> flowGraphs, ExecutorService executorService)
diff --git a/src/main/java/com/android/tools/r8/utils/DepthFirstSearchWorkListBase.java b/src/main/java/com/android/tools/r8/utils/DepthFirstSearchWorkListBase.java
index a4d1c8f..48ceda2 100644
--- a/src/main/java/com/android/tools/r8/utils/DepthFirstSearchWorkListBase.java
+++ b/src/main/java/com/android/tools/r8/utils/DepthFirstSearchWorkListBase.java
@@ -255,11 +255,12 @@
protected TraversalContinuation<TB, S> internalOnJoin(DFSNodeWithStateImpl<N, S> node) {
return joiner(
node,
- childStateMap.computeIfAbsent(
+ MapUtils.removeOrComputeDefault(
+ childStateMap,
node,
n -> {
assert false : "Unexpected joining of not visited node";
- return new ArrayList<>();
+ return Collections.emptyList();
}));
}
diff --git a/src/main/java/com/android/tools/r8/utils/MapUtils.java b/src/main/java/com/android/tools/r8/utils/MapUtils.java
index ac5d7f6..99fae2e 100644
--- a/src/main/java/com/android/tools/r8/utils/MapUtils.java
+++ b/src/main/java/com/android/tools/r8/utils/MapUtils.java
@@ -96,6 +96,11 @@
return value != null ? value : defaultValue;
}
+ public static <K, V> V removeOrComputeDefault(Map<K, V> map, K key, Function<K, V> defaultValue) {
+ V value = map.remove(key);
+ return value != null ? value : defaultValue.apply(key);
+ }
+
public static String toString(Map<?, ?> map) {
return StringUtils.join(
",", map.entrySet(), entry -> entry.getKey() + ":" + entry.getValue(), BraceType.TUBORG);
diff --git a/src/test/java/com/android/tools/r8/optimize/argumentpropagation/DefaultFieldValueAnalysisWithKeptSubclassTest.java b/src/test/java/com/android/tools/r8/optimize/argumentpropagation/DefaultFieldValueAnalysisWithKeptSubclassTest.java
index d2e241e..dd9a582 100644
--- a/src/test/java/com/android/tools/r8/optimize/argumentpropagation/DefaultFieldValueAnalysisWithKeptSubclassTest.java
+++ b/src/test/java/com/android/tools/r8/optimize/argumentpropagation/DefaultFieldValueAnalysisWithKeptSubclassTest.java
@@ -3,7 +3,6 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8.optimize.argumentpropagation;
-import static com.android.tools.r8.utils.codeinspector.Matchers.isAbsent;
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
import static org.hamcrest.MatcherAssert.assertThat;
@@ -65,11 +64,10 @@
inspector -> {
ClassSubject aClassSubject = inspector.clazz(A.class);
assertThat(aClassSubject, isPresent());
- assertThat(aClassSubject.uniqueFieldWithOriginalName("f"), isAbsent());
+ assertThat(aClassSubject.uniqueFieldWithOriginalName("f"), isPresent());
})
- // TODO(b/379034741): Should succeed with expected output.
.run(parameters.getRuntime(), Main.class, B.class.getTypeName())
- .assertSuccessWithEmptyOutput();
+ .assertSuccessWithOutputLines("Hello, world!");
}
static class Main {
diff --git a/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/MergedFieldTypeTest.java b/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/MergedFieldTypeTest.java
index 693a077..181a85a 100644
--- a/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/MergedFieldTypeTest.java
+++ b/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/MergedFieldTypeTest.java
@@ -118,7 +118,7 @@
ClassSubject testClassSubject = inspector.clazz(TestClass.class);
assertThat(testClassSubject, isPresent());
- if (enableVerticalClassMerging && parameters.canInitNewInstanceUsingSuperclassConstructor()) {
+ if (enableVerticalClassMerging) {
// Verify that TestClass.field has been removed.
assertEquals(1, testClassSubject.allFields().size());
