Revert "Stop relying on $VALUES name"
This reverts commit dff97021b297a11433530c80a1416b53c07fe9bf.
Reason for revert: b/178496619
Change-Id: I22c53a1cedc4c7a32c283a7919581fc1f58dcdd9
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 30ac84b..e775ac6 100644
--- a/src/main/java/com/android/tools/r8/graph/DexItemFactory.java
+++ b/src/main/java/com/android/tools/r8/graph/DexItemFactory.java
@@ -293,11 +293,6 @@
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");
@@ -1403,17 +1398,21 @@
return field == nameField || field == ordinalField;
}
- public boolean isEnumField(DexEncodedField staticField, DexType enumType) {
- assert staticField.isStatic();
- return staticField.getType() == enumType && staticField.isEnum() && staticField.isFinal();
+ 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 isValuesFieldCandidate(DexEncodedField staticField, DexType enumType) {
- assert staticField.isStatic();
- return staticField.getType().isArrayType()
- && staticField.getType().toArrayElementType(DexItemFactory.this) == enumType
- && staticField.isSynthetic()
- && 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;
}
}
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 fc01602..281215f 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,6 +12,9 @@
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;
@@ -23,7 +26,6 @@
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;
@@ -38,13 +40,10 @@
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) {
@@ -64,7 +63,7 @@
timing.begin("Analyze class initializer");
StaticFieldValues result =
new StaticFieldValueAnalysis(appView.withLiveness(), code, feedback)
- .analyze(classInitializerDefaultsResult);
+ .analyze(classInitializerDefaultsResult, code.context().getHolderType());
timing.end();
return result;
}
@@ -79,7 +78,8 @@
return this;
}
- StaticFieldValues analyze(ClassInitializerDefaultsResult classInitializerDefaultsResult) {
+ StaticFieldValues analyze(
+ ClassInitializerDefaultsResult classInitializerDefaultsResult, DexType holderType) {
computeFieldOptimizationInfo(classInitializerDefaultsResult);
return builder.build();
}
@@ -218,41 +218,16 @@
return null;
}
assert !value.hasAliasedValue();
- if (value.isPhi()) {
- return null;
- }
- if (value.definition.isNewArrayEmpty()) {
+ if (isEnumValuesArray(value)) {
return computeSingleEnumFieldValueForValuesArray(value);
}
- if (value.definition.isNewInstance()) {
- return computeSingleEnumFieldValueForInstance(value);
- }
- return null;
+ return computeSingleEnumFieldValueForInstance(value);
}
private SingleFieldValue computeSingleEnumFieldValueForValuesArray(Value value) {
- if (!value.definition.isNewArrayEmpty()) {
+ if (!value.isDefinedByInstructionSatisfying(Instruction::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) {
@@ -292,9 +267,7 @@
// We need the state of all fields for the analysis to be valuable.
return null;
}
- if (!valuesArrayIndexMatchesOrdinal(index, objectState)) {
- return null;
- }
+ assert verifyValuesArrayIndexMatchesOrdinal(index, objectState);
if (valuesState[index] != null) {
return null;
}
@@ -353,25 +326,24 @@
return ObjectState.empty();
}
- private boolean valuesArrayIndexMatchesOrdinal(int ordinal, ObjectState objectState) {
+ private boolean verifyValuesArrayIndexMatchesOrdinal(int ordinal, ObjectState objectState) {
DexEncodedField ordinalField =
appView
.appInfo()
.resolveField(appView.dexItemFactory().enumMembers.ordinalField, context)
.getResolvedField();
- if (ordinalField == null) {
- return false;
- }
+ assert ordinalField != null;
AbstractValue ordinalState = objectState.getAbstractFieldValue(ordinalField);
- if (ordinalState == null || !ordinalState.isSingleNumberValue()) {
- return false;
- }
- int intValue = ordinalState.asSingleNumberValue().getIntValue();
- return intValue == ordinal;
+ assert ordinalState != null;
+ assert ordinalState.isSingleNumberValue();
+ assert ordinalState.asSingleNumberValue().getIntValue() == ordinal;
+ return true;
}
private SingleFieldValue computeSingleEnumFieldValueForInstance(Value value) {
- assert value.isDefinedByInstructionSatisfying(Instruction::isNewInstance);
+ if (!value.isDefinedByInstructionSatisfying(Instruction::isNewInstance)) {
+ return null;
+ }
NewInstance newInstance = value.definition.asNewInstance();
// Some enums have direct subclasses, and the subclass is instantiated here.
@@ -487,7 +459,30 @@
}
private boolean isEnumValuesArray(Value value) {
- SingleFieldValue singleFieldValue = computeSingleEnumFieldValueForValuesArray(value);
- return singleFieldValue != null && singleFieldValue.getState().isEnumValuesObjectState();
+ 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;
}
}
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 65e1ea3..b2dcdcd 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,10 +37,17 @@
// 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, ObjectState> enumAbstractValues;
+ private final ImmutableMap<DexField, AbstractValue> enumAbstractValues;
+ private final DexField valuesField;
+ private final AbstractValue valuesAbstractValue;
- public EnumStaticFieldValues(ImmutableMap<DexField, ObjectState> enumAbstractValues) {
+ public EnumStaticFieldValues(
+ ImmutableMap<DexField, AbstractValue> enumAbstractValues,
+ DexField valuesField,
+ AbstractValue valuesAbstractValue) {
this.enumAbstractValues = enumAbstractValues;
+ this.valuesField = valuesField;
+ this.valuesAbstractValue = valuesAbstractValue;
}
static StaticFieldValues.Builder builder() {
@@ -48,38 +55,33 @@
}
public static class Builder extends StaticFieldValues.Builder {
- private final ImmutableMap.Builder<DexField, ObjectState> enumObjectStateBuilder =
+ private final ImmutableMap.Builder<DexField, AbstractValue> enumAbstractValuesBuilder =
ImmutableMap.builder();
- private AbstractValue valuesCandidateAbstractValue;
+ private DexField valuesFields;
+ private AbstractValue valuesAbstractValue;
Builder() {}
@Override
public void recordStaticField(
DexEncodedField staticField, AbstractValue value, DexItemFactory factory) {
- 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());
- }
+ // 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);
}
}
@Override
public StaticFieldValues build() {
- ImmutableMap<DexField, ObjectState> enumAbstractValues = enumObjectStateBuilder.build();
- if (enumAbstractValues.isEmpty()) {
+ ImmutableMap<DexField, AbstractValue> enumAbstractValues =
+ enumAbstractValuesBuilder.build();
+ if (valuesAbstractValue == null && enumAbstractValues.isEmpty()) {
return EmptyStaticValues.getInstance();
}
- assert enumAbstractValues.values().stream().noneMatch(ObjectState::isEmpty);
- return new EnumStaticFieldValues(enumAbstractValues);
+ return new EnumStaticFieldValues(enumAbstractValues, valuesFields, valuesAbstractValue);
}
}
@@ -94,7 +96,20 @@
}
public ObjectState getObjectStateForPossiblyPinnedField(DexField field) {
- return enumAbstractValues.get(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;
}
}
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 cf0ecb5..166022e 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,36 +491,34 @@
// Step 1: We iterate over the field to find direct enum instance information and the values
// fields.
for (DexEncodedField staticField : enumClass.staticFields()) {
- if (factory.enumMembers.isEnumField(staticField, enumClass.type)) {
+ if (EnumUnboxingCandidateAnalysis.isEnumField(staticField, enumClass.type)) {
ObjectState enumState =
enumStaticFieldValues.getObjectStateForPossiblyPinnedField(staticField.field);
- if (enumState == null) {
- if (!isFinalFieldInitialized(staticField, enumClass)) {
- continue;
+ if (enumState != null) {
+ OptionalInt optionalOrdinal = getOrdinal(enumState);
+ if (!optionalOrdinal.isPresent()) {
+ return null;
}
- // Tracking the content of the field yielded either an empty object state, or something
- // incoherent. We bail out.
+ 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()) {
return null;
}
- OptionalInt optionalOrdinal = getOrdinal(enumState);
- if (!optionalOrdinal.isPresent()) {
+ assert valuesValue.isSingleFieldValue();
+ ObjectState valuesState = valuesValue.asSingleFieldValue().getState();
+ if (!valuesState.isEnumValuesObjectState()) {
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();
@@ -566,13 +564,6 @@
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 4c7581e..6c20c4f 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,
- "$$values$$field$" + compatibleName(enumType));
+ factory.enumValuesFieldName + "$field$" + compatibleName(enumType));
}
private DexEncodedField computeValuesEncodedField(DexField field) {
@@ -451,7 +451,7 @@
return factory.createMethod(
relocator.getNewMemberLocationFor(enumType),
factory.createProto(factory.intArrayType),
- "$$values$$method$" + compatibleName(enumType));
+ factory.enumValuesFieldName + "$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 b21cbd0..89395ef 100644
--- a/src/test/java/com/android/tools/r8/enumunboxing/EnumToStringLibTest.java
+++ b/src/test/java/com/android/tools/r8/enumunboxing/EnumToStringLibTest.java
@@ -97,6 +97,13 @@
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(
