Stop relying on $VALUES name
Bug: 160535629
Change-Id: I379f4c548426b9f10753f7b18612edd77e57926c
diff --git a/src/main/java/com/android/tools/r8/graph/DexItemFactory.java b/src/main/java/com/android/tools/r8/graph/DexItemFactory.java
index e6d824a..e2845c4 100644
--- a/src/main/java/com/android/tools/r8/graph/DexItemFactory.java
+++ b/src/main/java/com/android/tools/r8/graph/DexItemFactory.java
@@ -292,6 +292,11 @@
createString(Constants.TEMPORARY_INSTANCE_INITIALIZER_PREFIX);
public final DexString thisName = createString("this");
+
+ // As much as possible, R8 should rely on the content of the static enum field, using
+ // enumMembers.isValuesFieldCandidate or checking the object state in the optimization info.
+ // The field name is unrealiable since the filed can be minified prior to this compilation.
+ // We keep enumValuesFieldName as a heuristic only.
public final DexString enumValuesFieldName = createString("$VALUES");
public final DexString enabledFieldName = createString("ENABLED");
@@ -1380,21 +1385,17 @@
return field == nameField || field == ordinalField;
}
- public boolean isValuesMethod(DexMethod method, DexClass enumClass) {
- assert enumClass.isEnum();
- return method.holder == enumClass.type
- && method.proto.returnType == enumClass.type.toArrayType(1, DexItemFactory.this)
- && method.proto.parameters.size() == 0
- && method.name == valuesMethodName;
+ public boolean isEnumField(DexEncodedField staticField, DexType enumType) {
+ assert staticField.isStatic();
+ return staticField.getType() == enumType && staticField.isEnum() && staticField.isFinal();
}
- public boolean isValueOfMethod(DexMethod method, DexClass enumClass) {
- assert enumClass.isEnum();
- return method.holder == enumClass.type
- && method.proto.returnType == enumClass.type
- && method.proto.parameters.size() == 1
- && method.proto.parameters.values[0] == stringType
- && method.name == valueOfMethodName;
+ public boolean isValuesFieldCandidate(DexEncodedField staticField, DexType enumType) {
+ assert staticField.isStatic();
+ return staticField.getType().isArrayType()
+ && staticField.getType().toArrayElementType(DexItemFactory.this) == enumType
+ && staticField.isSynthetic()
+ && staticField.isFinal();
}
}
diff --git a/src/main/java/com/android/tools/r8/ir/analysis/fieldvalueanalysis/StaticFieldValueAnalysis.java b/src/main/java/com/android/tools/r8/ir/analysis/fieldvalueanalysis/StaticFieldValueAnalysis.java
index 281215f..fc01602 100644
--- a/src/main/java/com/android/tools/r8/ir/analysis/fieldvalueanalysis/StaticFieldValueAnalysis.java
+++ b/src/main/java/com/android/tools/r8/ir/analysis/fieldvalueanalysis/StaticFieldValueAnalysis.java
@@ -12,9 +12,6 @@
import com.android.tools.r8.graph.DexClass;
import com.android.tools.r8.graph.DexClassAndMethod;
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.DexType;
import com.android.tools.r8.graph.DexValue;
import com.android.tools.r8.graph.DexValue.DexValueNull;
import com.android.tools.r8.ir.analysis.type.ClassTypeElement;
@@ -26,6 +23,7 @@
import com.android.tools.r8.ir.analysis.value.NullOrAbstractValue;
import com.android.tools.r8.ir.analysis.value.ObjectState;
import com.android.tools.r8.ir.analysis.value.SingleFieldValue;
+import com.android.tools.r8.ir.analysis.value.UnknownValue;
import com.android.tools.r8.ir.code.ArrayPut;
import com.android.tools.r8.ir.code.FieldInstruction;
import com.android.tools.r8.ir.code.IRCode;
@@ -40,10 +38,13 @@
import com.android.tools.r8.ir.optimize.info.field.InstanceFieldInitializationInfoCollection;
import com.android.tools.r8.shaking.AppInfoWithLiveness;
import com.android.tools.r8.utils.Timing;
+import java.util.IdentityHashMap;
+import java.util.Map;
public class StaticFieldValueAnalysis extends FieldValueAnalysis {
private final StaticFieldValues.Builder builder;
+ private final Map<Value, AbstractValue> computedValues = new IdentityHashMap<>();
private StaticFieldValueAnalysis(
AppView<AppInfoWithLiveness> appView, IRCode code, OptimizationFeedback feedback) {
@@ -63,7 +64,7 @@
timing.begin("Analyze class initializer");
StaticFieldValues result =
new StaticFieldValueAnalysis(appView.withLiveness(), code, feedback)
- .analyze(classInitializerDefaultsResult, code.context().getHolderType());
+ .analyze(classInitializerDefaultsResult);
timing.end();
return result;
}
@@ -78,8 +79,7 @@
return this;
}
- StaticFieldValues analyze(
- ClassInitializerDefaultsResult classInitializerDefaultsResult, DexType holderType) {
+ StaticFieldValues analyze(ClassInitializerDefaultsResult classInitializerDefaultsResult) {
computeFieldOptimizationInfo(classInitializerDefaultsResult);
return builder.build();
}
@@ -218,16 +218,41 @@
return null;
}
assert !value.hasAliasedValue();
- if (isEnumValuesArray(value)) {
+ if (value.isPhi()) {
+ return null;
+ }
+ if (value.definition.isNewArrayEmpty()) {
return computeSingleEnumFieldValueForValuesArray(value);
}
- return computeSingleEnumFieldValueForInstance(value);
+ if (value.definition.isNewInstance()) {
+ return computeSingleEnumFieldValueForInstance(value);
+ }
+ return null;
}
private SingleFieldValue computeSingleEnumFieldValueForValuesArray(Value value) {
- if (!value.isDefinedByInstructionSatisfying(Instruction::isNewArrayEmpty)) {
+ if (!value.definition.isNewArrayEmpty()) {
return null;
}
+ AbstractValue valuesValue = computedValues.get(value);
+ if (valuesValue != null) {
+ // This implicitely answers null if the value could not get computed.
+ if (valuesValue.isSingleFieldValue()) {
+ SingleFieldValue fieldValue = valuesValue.asSingleFieldValue();
+ if (fieldValue.getState().isEnumValuesObjectState()) {
+ return fieldValue;
+ }
+ }
+ return null;
+ }
+ SingleFieldValue singleFieldValue = internalComputeSingleEnumFieldValueForValuesArray(value);
+ computedValues.put(
+ value, singleFieldValue == null ? UnknownValue.getInstance() : singleFieldValue);
+ return singleFieldValue;
+ }
+
+ private SingleFieldValue internalComputeSingleEnumFieldValueForValuesArray(Value value) {
+ assert value.isDefinedByInstructionSatisfying(Instruction::isNewArrayEmpty);
NewArrayEmpty newArrayEmpty = value.definition.asNewArrayEmpty();
if (newArrayEmpty.type.toBaseType(appView.dexItemFactory()) != context.getHolder().type) {
@@ -267,7 +292,9 @@
// We need the state of all fields for the analysis to be valuable.
return null;
}
- assert verifyValuesArrayIndexMatchesOrdinal(index, objectState);
+ if (!valuesArrayIndexMatchesOrdinal(index, objectState)) {
+ return null;
+ }
if (valuesState[index] != null) {
return null;
}
@@ -326,24 +353,25 @@
return ObjectState.empty();
}
- private boolean verifyValuesArrayIndexMatchesOrdinal(int ordinal, ObjectState objectState) {
+ private boolean valuesArrayIndexMatchesOrdinal(int ordinal, ObjectState objectState) {
DexEncodedField ordinalField =
appView
.appInfo()
.resolveField(appView.dexItemFactory().enumMembers.ordinalField, context)
.getResolvedField();
- assert ordinalField != null;
+ if (ordinalField == null) {
+ return false;
+ }
AbstractValue ordinalState = objectState.getAbstractFieldValue(ordinalField);
- assert ordinalState != null;
- assert ordinalState.isSingleNumberValue();
- assert ordinalState.asSingleNumberValue().getIntValue() == ordinal;
- return true;
+ if (ordinalState == null || !ordinalState.isSingleNumberValue()) {
+ return false;
+ }
+ int intValue = ordinalState.asSingleNumberValue().getIntValue();
+ return intValue == ordinal;
}
private SingleFieldValue computeSingleEnumFieldValueForInstance(Value value) {
- if (!value.isDefinedByInstructionSatisfying(Instruction::isNewInstance)) {
- return null;
- }
+ assert value.isDefinedByInstructionSatisfying(Instruction::isNewInstance);
NewInstance newInstance = value.definition.asNewInstance();
// Some enums have direct subclasses, and the subclass is instantiated here.
@@ -459,30 +487,7 @@
}
private boolean isEnumValuesArray(Value value) {
- assert context.getHolder().isEnum();
- DexItemFactory dexItemFactory = appView.dexItemFactory();
- DexField valuesField =
- dexItemFactory.createField(
- context.getHolderType(),
- context.getHolderType().toArrayType(1, dexItemFactory),
- dexItemFactory.enumValuesFieldName);
-
- Value root = value.getAliasedValue();
- if (root.isPhi()) {
- return false;
- }
-
- Instruction definition = root.definition;
- if (definition.isNewArrayEmpty()) {
- for (Instruction user : root.aliasedUsers()) {
- if (user.isStaticPut() && user.asStaticPut().getField() == valuesField) {
- return true;
- }
- }
- } else if (definition.isStaticGet()) {
- return definition.asStaticGet().getField() == valuesField;
- }
-
- return false;
+ SingleFieldValue singleFieldValue = computeSingleEnumFieldValueForValuesArray(value);
+ return singleFieldValue != null && singleFieldValue.getState().isEnumValuesObjectState();
}
}
diff --git a/src/main/java/com/android/tools/r8/ir/analysis/fieldvalueanalysis/StaticFieldValues.java b/src/main/java/com/android/tools/r8/ir/analysis/fieldvalueanalysis/StaticFieldValues.java
index b2dcdcd..65e1ea3 100644
--- a/src/main/java/com/android/tools/r8/ir/analysis/fieldvalueanalysis/StaticFieldValues.java
+++ b/src/main/java/com/android/tools/r8/ir/analysis/fieldvalueanalysis/StaticFieldValues.java
@@ -37,17 +37,10 @@
// All the abstract values stored here may match a pinned field, using them requires therefore
// to check the field is not pinned or prove it is no longer pinned.
public static class EnumStaticFieldValues extends StaticFieldValues {
- private final ImmutableMap<DexField, AbstractValue> enumAbstractValues;
- private final DexField valuesField;
- private final AbstractValue valuesAbstractValue;
+ private final ImmutableMap<DexField, ObjectState> enumAbstractValues;
- public EnumStaticFieldValues(
- ImmutableMap<DexField, AbstractValue> enumAbstractValues,
- DexField valuesField,
- AbstractValue valuesAbstractValue) {
+ public EnumStaticFieldValues(ImmutableMap<DexField, ObjectState> enumAbstractValues) {
this.enumAbstractValues = enumAbstractValues;
- this.valuesField = valuesField;
- this.valuesAbstractValue = valuesAbstractValue;
}
static StaticFieldValues.Builder builder() {
@@ -55,33 +48,38 @@
}
public static class Builder extends StaticFieldValues.Builder {
- private final ImmutableMap.Builder<DexField, AbstractValue> enumAbstractValuesBuilder =
+ private final ImmutableMap.Builder<DexField, ObjectState> enumObjectStateBuilder =
ImmutableMap.builder();
- private DexField valuesFields;
- private AbstractValue valuesAbstractValue;
+ private AbstractValue valuesCandidateAbstractValue;
Builder() {}
@Override
public void recordStaticField(
DexEncodedField staticField, AbstractValue value, DexItemFactory factory) {
- // TODO(b/166532388): Stop relying on the values name.
- if (staticField.getName() == factory.enumValuesFieldName) {
- valuesFields = staticField.field;
- valuesAbstractValue = value;
- } else if (staticField.isEnum()) {
- enumAbstractValuesBuilder.put(staticField.field, value);
+ if (factory.enumMembers.isValuesFieldCandidate(staticField, staticField.getHolderType())) {
+ if (value.isSingleFieldValue()
+ && value.asSingleFieldValue().getState().isEnumValuesObjectState()) {
+ assert valuesCandidateAbstractValue == null
+ || valuesCandidateAbstractValue.equals(value);
+ valuesCandidateAbstractValue = value;
+ enumObjectStateBuilder.put(staticField.field, value.asSingleFieldValue().getState());
+ }
+ } else if (factory.enumMembers.isEnumField(staticField, staticField.getHolderType())) {
+ if (value.isSingleFieldValue() && !value.asSingleFieldValue().getState().isEmpty()) {
+ enumObjectStateBuilder.put(staticField.field, value.asSingleFieldValue().getState());
+ }
}
}
@Override
public StaticFieldValues build() {
- ImmutableMap<DexField, AbstractValue> enumAbstractValues =
- enumAbstractValuesBuilder.build();
- if (valuesAbstractValue == null && enumAbstractValues.isEmpty()) {
+ ImmutableMap<DexField, ObjectState> enumAbstractValues = enumObjectStateBuilder.build();
+ if (enumAbstractValues.isEmpty()) {
return EmptyStaticValues.getInstance();
}
- return new EnumStaticFieldValues(enumAbstractValues, valuesFields, valuesAbstractValue);
+ assert enumAbstractValues.values().stream().noneMatch(ObjectState::isEmpty);
+ return new EnumStaticFieldValues(enumAbstractValues);
}
}
@@ -96,20 +94,7 @@
}
public ObjectState getObjectStateForPossiblyPinnedField(DexField field) {
- AbstractValue fieldValue = enumAbstractValues.get(field);
- if (fieldValue == null || fieldValue.isZero()) {
- return null;
- }
- if (fieldValue.isSingleFieldValue()) {
- return fieldValue.asSingleFieldValue().getState();
- }
- assert fieldValue.isUnknown();
- return ObjectState.empty();
- }
-
- public AbstractValue getValuesAbstractValueForPossiblyPinnedField(DexField field) {
- assert valuesField == field || valuesAbstractValue == null;
- return valuesAbstractValue;
+ return enumAbstractValues.get(field);
}
}
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxer.java b/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxer.java
index 166022e..cf0ecb5 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxer.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxer.java
@@ -491,34 +491,36 @@
// Step 1: We iterate over the field to find direct enum instance information and the values
// fields.
for (DexEncodedField staticField : enumClass.staticFields()) {
- if (EnumUnboxingCandidateAnalysis.isEnumField(staticField, enumClass.type)) {
+ if (factory.enumMembers.isEnumField(staticField, enumClass.type)) {
ObjectState enumState =
enumStaticFieldValues.getObjectStateForPossiblyPinnedField(staticField.field);
- if (enumState != null) {
- OptionalInt optionalOrdinal = getOrdinal(enumState);
- if (!optionalOrdinal.isPresent()) {
- return null;
+ if (enumState == null) {
+ if (!isFinalFieldInitialized(staticField, enumClass)) {
+ continue;
}
- int ordinal = optionalOrdinal.getAsInt();
- unboxedValues.put(staticField.field, ordinalToUnboxedInt(ordinal));
- ordinalToObjectState.put(ordinal, enumState);
- }
- } else if (EnumUnboxingCandidateAnalysis.matchesValuesField(
- staticField, enumClass.type, factory)) {
- AbstractValue valuesValue =
- enumStaticFieldValues.getValuesAbstractValueForPossiblyPinnedField(staticField.field);
- if (valuesValue == null || valuesValue.isZero()) {
- // Unused field
- continue;
- }
- if (valuesValue.isUnknown()) {
+ // Tracking the content of the field yielded either an empty object state, or something
+ // incoherent. We bail out.
return null;
}
- assert valuesValue.isSingleFieldValue();
- ObjectState valuesState = valuesValue.asSingleFieldValue().getState();
- if (!valuesState.isEnumValuesObjectState()) {
+ OptionalInt optionalOrdinal = getOrdinal(enumState);
+ if (!optionalOrdinal.isPresent()) {
return null;
}
+ int ordinal = optionalOrdinal.getAsInt();
+ unboxedValues.put(staticField.field, ordinalToUnboxedInt(ordinal));
+ ordinalToObjectState.put(ordinal, enumState);
+ } else if (factory.enumMembers.isValuesFieldCandidate(staticField, enumClass.type)) {
+ ObjectState valuesState =
+ enumStaticFieldValues.getObjectStateForPossiblyPinnedField(staticField.field);
+ if (valuesState == null) {
+ if (!isFinalFieldInitialized(staticField, enumClass)) {
+ continue;
+ }
+ // We could not track the content of that field, and the field could be a values field.
+ // We conservatively bail out.
+ return null;
+ }
+ assert valuesState.isEnumValuesObjectState();
assert valuesContents == null
|| valuesContents.equals(valuesState.asEnumValuesObjectState());
valuesContents = valuesState.asEnumValuesObjectState();
@@ -564,6 +566,13 @@
valuesContents == null ? EnumData.INVALID_VALUES_SIZE : valuesContents.getEnumValuesSize());
}
+ private boolean isFinalFieldInitialized(DexEncodedField staticField, DexProgramClass enumClass) {
+ assert staticField.isFinal();
+ return appView
+ .appInfo()
+ .isFieldOnlyWrittenInMethodIgnoringPinning(staticField, enumClass.getClassInitializer());
+ }
+
private EnumInstanceFieldData computeEnumFieldData(
DexField instanceField,
DexProgramClass enumClass,
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxingRewriter.java b/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxingRewriter.java
index 6c20c4f..4c7581e 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxingRewriter.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxingRewriter.java
@@ -434,7 +434,7 @@
return factory.createField(
relocator.getNewMemberLocationFor(enumType),
factory.intArrayType,
- factory.enumValuesFieldName + "$field$" + compatibleName(enumType));
+ "$$values$$field$" + compatibleName(enumType));
}
private DexEncodedField computeValuesEncodedField(DexField field) {
@@ -451,7 +451,7 @@
return factory.createMethod(
relocator.getNewMemberLocationFor(enumType),
factory.createProto(factory.intArrayType),
- factory.enumValuesFieldName + "$method$" + compatibleName(enumType));
+ "$$values$$method$" + compatibleName(enumType));
}
private DexEncodedMethod computeValuesEncodedMethod(
diff --git a/src/test/java/com/android/tools/r8/enumunboxing/EnumToStringLibTest.java b/src/test/java/com/android/tools/r8/enumunboxing/EnumToStringLibTest.java
index 89395ef..b21cbd0 100644
--- a/src/test/java/com/android/tools/r8/enumunboxing/EnumToStringLibTest.java
+++ b/src/test/java/com/android/tools/r8/enumunboxing/EnumToStringLibTest.java
@@ -97,13 +97,6 @@
return testForR8(Backend.CF)
.addProgramClasses(ToStringLib.class, ToStringLib.LibEnum.class)
.addKeepRules(enumKeepRules.getKeepRules())
- // TODO(b/160535629): Work-around on some optimizations relying on $VALUES name.
- .addKeepRules(
- "-keep enum "
- + ToStringLib.LibEnum.class.getName()
- + " { static "
- + ToStringLib.LibEnum.class.getName()
- + "[] $VALUES; }")
.addOptionsModification(opt -> enableEnumOptions(opt, enumValueOptimization))
.addKeepMethodRules(
Reference.methodFromMethod(
