Allow enums with subtypes without enum flag to be unboxed Bug: b/315186101 Change-Id: I98d002594af56c2f63e953d1236652f3d3c882f8
diff --git a/src/main/java/com/android/tools/r8/graph/ClassAccessFlags.java b/src/main/java/com/android/tools/r8/graph/ClassAccessFlags.java index dba763b..888f1e8 100644 --- a/src/main/java/com/android/tools/r8/graph/ClassAccessFlags.java +++ b/src/main/java/com/android/tools/r8/graph/ClassAccessFlags.java
@@ -184,6 +184,10 @@ set(Constants.ACC_ENUM); } + public void unsetEnum() { + unset(Constants.ACC_ENUM); + } + public boolean isRecord() { return isSet(Constants.ACC_RECORD); }
diff --git a/src/main/java/com/android/tools/r8/graph/ObjectAllocationInfoCollectionImpl.java b/src/main/java/com/android/tools/r8/graph/ObjectAllocationInfoCollectionImpl.java index 0eb2d7e..6050c9d 100644 --- a/src/main/java/com/android/tools/r8/graph/ObjectAllocationInfoCollectionImpl.java +++ b/src/main/java/com/android/tools/r8/graph/ObjectAllocationInfoCollectionImpl.java
@@ -490,7 +490,6 @@ (clazz, allocationSitesForClass) -> { DexType type = lens.lookupType(clazz.type); if (type.isPrimitiveType()) { - assert clazz.isEnum(); return; } DexProgramClass rewrittenClass = asProgramClassOrNull(definitions.definitionFor(type));
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxerImpl.java b/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxerImpl.java index 5132141..373c579 100644 --- a/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxerImpl.java +++ b/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxerImpl.java
@@ -1417,12 +1417,12 @@ DexClassAndMethod mostAccurateTarget = singleTarget == null ? resolvedMethod : singleTarget; if (mostAccurateTarget.isProgramMethod()) { - if (mostAccurateTarget.getHolder().isEnum() - && resolvedMethod.getDefinition().isInstanceInitializer()) { + DexProgramClass candidateOrNull = + getEnumUnboxingCandidateOrNull(mostAccurateTarget.getHolderType()); + if (candidateOrNull != null && resolvedMethod.getDefinition().isInstanceInitializer()) { // The enum instance initializer is only allowed to be called from an initializer of the // enum itself. - if (getEnumUnboxingCandidateOrNull(code.context().getHolderType()) - != getEnumUnboxingCandidateOrNull(mostAccurateTarget.getHolderType()) + if (getEnumUnboxingCandidateOrNull(code.context().getHolderType()) != candidateOrNull || !context.getDefinition().isInitializer()) { return Reason.INVALID_INIT; }
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxingCandidateAnalysis.java b/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxingCandidateAnalysis.java index 86709f7..c0a7655 100644 --- a/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxingCandidateAnalysis.java +++ b/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxingCandidateAnalysis.java
@@ -4,7 +4,6 @@ package com.android.tools.r8.ir.optimize.enums; -import static com.android.tools.r8.utils.MapUtils.ignoreKey; import com.android.tools.r8.graph.AppView; import com.android.tools.r8.graph.DexClass; @@ -14,16 +13,14 @@ import com.android.tools.r8.graph.DexProgramClass; import com.android.tools.r8.graph.DexProto; import com.android.tools.r8.graph.DexType; +import com.android.tools.r8.graph.ImmediateProgramSubtypingInfo; import com.android.tools.r8.graph.lens.GraphLens; import com.android.tools.r8.ir.optimize.enums.eligibility.Reason; import com.android.tools.r8.shaking.AppInfoWithLiveness; import com.android.tools.r8.shaking.KeepInfoCollection; import com.android.tools.r8.utils.InternalOptions; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Sets; -import java.util.IdentityHashMap; -import java.util.Map; -import java.util.Set; +import java.util.List; class EnumUnboxingCandidateAnalysis { @@ -35,12 +32,9 @@ private final AppView<AppInfoWithLiveness> appView; private final EnumUnboxerImpl enumUnboxer; private final DexItemFactory factory; - private EnumUnboxingCandidateInfoCollection enumToUnboxCandidates = + private final EnumUnboxingCandidateInfoCollection enumToUnboxCandidates = new EnumUnboxingCandidateInfoCollection(); - private final Map<DexType, Set<DexProgramClass>> enumSubclasses = new IdentityHashMap<>(); - private final Set<DexType> ineligibleCandidates = Sets.newIdentityHashSet(); - EnumUnboxingCandidateAnalysis(AppView<AppInfoWithLiveness> appView, EnumUnboxerImpl enumUnboxer) { this.appView = appView; this.enumUnboxer = enumUnboxer; @@ -54,13 +48,13 @@ // disables the enum unboxer. return enumToUnboxCandidates; } + ImmediateProgramSubtypingInfo subtypingInfo = ImmediateProgramSubtypingInfo.create(appView); + // In recent Kotlin enum subclasses are not flagged as enum, so we cannot rely on the flag. for (DexProgramClass clazz : appView.appInfo().classes()) { - if (clazz.isEnum()) { - analyzeEnum(graphLensForPrimaryOptimizationPass, clazz); + if (clazz.isEnum() && clazz.superType.isIdenticalTo(factory.enumType)) { + analyzeEnum(graphLensForPrimaryOptimizationPass, clazz, subtypingInfo); } } - removeIneligibleCandidates(); - setEnumSubclassesOnCandidates(); removeEnumsInAnnotations(); removePinnedCandidates(); if (appView.options().protoShrinking().isProtoShrinkingEnabled()) { @@ -70,42 +64,28 @@ return enumToUnboxCandidates; } - private void setEnumSubclassesOnCandidates() { - enumToUnboxCandidates.forEachCandidateInfo( - info -> { - DexType type = info.getEnumClass().getType(); - enumToUnboxCandidates.setEnumSubclasses( - type, enumSubclasses.getOrDefault(type, ImmutableSet.of())); - }); - } - - private void removeIneligibleCandidates() { - for (DexType ineligibleCandidate : ineligibleCandidates) { - enumToUnboxCandidates.removeCandidate(ineligibleCandidate); + private void analyzeEnum( + GraphLens graphLensForPrimaryOptimizationPass, + DexProgramClass clazz, + ImmediateProgramSubtypingInfo subtypingInfo) { + if (!isSuperEnumUnboxingCandidate(clazz)) { + return; } - } - @SuppressWarnings("ReferenceEquality") - private void analyzeEnum(GraphLens graphLensForPrimaryOptimizationPass, DexProgramClass clazz) { - if (clazz.superType == factory.enumType) { - if (isSuperEnumUnboxingCandidate(clazz)) { - enumToUnboxCandidates.addCandidate(appView, clazz, graphLensForPrimaryOptimizationPass); + List<DexProgramClass> subtypes = subtypingInfo.getSubclasses(clazz); + ImmutableSet.Builder<DexProgramClass> subEnumClassesBuilder = ImmutableSet.builder(); + for (DexProgramClass subEnum : subtypes) { + if (!isSubEnumUnboxingCandidate(subEnum)) { + return; } - } else { - if (isSubEnumUnboxingCandidate(clazz) - && appView.options().testing.enableEnumWithSubtypesUnboxing) { - enumSubclasses - .computeIfAbsent(clazz.superType, ignoreKey(Sets::newIdentityHashSet)) - .add(clazz); - } else { - ineligibleCandidates.add(clazz.superType); - } + subEnumClassesBuilder.add(subEnum); } + enumToUnboxCandidates.addCandidate( + appView, clazz, subEnumClassesBuilder.build(), graphLensForPrimaryOptimizationPass); } @SuppressWarnings("ReferenceEquality") private boolean isSubEnumUnboxingCandidate(DexProgramClass clazz) { - assert clazz.isEnum(); boolean result = true; // Javac does not seem to generate enums with more than a single subtype level. // TODO(b/273910479): Stop using isEffectivelyFinal. @@ -132,29 +112,14 @@ return result; } - @SuppressWarnings("ReferenceEquality") private boolean isSuperEnumUnboxingCandidate(DexProgramClass clazz) { assert clazz.isEnum(); - - // This is used in debug mode, where we don't do quick returns to log all the reasons an enum - // is not unboxed. - boolean result = true; - - // TODO(b/271385332): Change this into an assert when legacy is removed. - if (clazz.superType != factory.enumType) { - if (!enumUnboxer.reportFailure(clazz, Reason.INVALID_LIBRARY_SUPERTYPE)) { - return false; - } - result = false; - } - + assert clazz.superType.isIdenticalTo(factory.enumType); if (clazz.instanceFields().size() > MAX_INSTANCE_FIELDS_FOR_UNBOXING) { - if (!enumUnboxer.reportFailure(clazz, Reason.MANY_INSTANCE_FIELDS)) { - return false; - } - result = false; + enumUnboxer.reportFailure(clazz, Reason.MANY_INSTANCE_FIELDS); + return false; } - return result; + return true; } private void removeEnumsInAnnotations() {
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxingCandidateInfoCollection.java b/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxingCandidateInfoCollection.java index aaa428d..c2444a8 100644 --- a/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxingCandidateInfoCollection.java +++ b/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxingCandidateInfoCollection.java
@@ -34,11 +34,16 @@ public void addCandidate( AppView<AppInfoWithLiveness> appView, DexProgramClass enumClass, + Set<DexProgramClass> subclasses, GraphLens graphLensForPrimaryOptimizationPass) { assert !enumTypeToInfo.containsKey(enumClass.type); enumTypeToInfo.put( enumClass.type, - new EnumUnboxingCandidateInfo(appView, enumClass, graphLensForPrimaryOptimizationPass)); + new EnumUnboxingCandidateInfo( + appView, enumClass, subclasses, graphLensForPrimaryOptimizationPass)); + for (DexProgramClass subclass : subclasses) { + subEnumToSuperEnumMap.put(subclass.getType(), enumClass.getType()); + } } public boolean hasSubtypes(DexType enumType) { @@ -50,13 +55,6 @@ enumTypeToInfo.get(enumType).getSubclasses(), DexClass::getType); } - public void setEnumSubclasses(DexType superEnum, Set<DexProgramClass> subclasses) { - enumTypeToInfo.get(superEnum).setSubclasses(subclasses); - for (DexProgramClass subclass : subclasses) { - subEnumToSuperEnumMap.put(subclass.getType(), superEnum); - } - } - public void addPrunedMethod(ProgramMethod method) { prunedMethods.add(method.getReference()); } @@ -174,10 +172,12 @@ public EnumUnboxingCandidateInfo( AppView<AppInfoWithLiveness> appView, DexProgramClass enumClass, + Set<DexProgramClass> subclasses, GraphLens graphLensForPrimaryOptimizationPass) { assert enumClass != null; assert appView.graphLens() == graphLensForPrimaryOptimizationPass; this.enumClass = enumClass; + this.subclasses = subclasses; this.methodDependencies = LongLivedProgramMethodSetBuilder.createConcurrentForIdentitySet( graphLensForPrimaryOptimizationPass); @@ -188,10 +188,6 @@ return subclasses; } - public void setSubclasses(Set<DexProgramClass> subclasses) { - this.subclasses = subclasses; - } - public DexProgramClass getEnumClass() { return enumClass; }
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/enums/eligibility/Reason.java b/src/main/java/com/android/tools/r8/ir/optimize/enums/eligibility/Reason.java index 56b05f6..3cf50f1 100644 --- a/src/main/java/com/android/tools/r8/ir/optimize/enums/eligibility/Reason.java +++ b/src/main/java/com/android/tools/r8/ir/optimize/enums/eligibility/Reason.java
@@ -11,18 +11,13 @@ public abstract class Reason { public static final Reason ELIGIBLE = new StringReason("ELIGIBLE"); - public static final Reason ACCESSIBILITY = new StringReason("ACCESSIBILITY"); public static final Reason ANNOTATION = new StringReason("ANNOTATION"); public static final Reason PINNED = new StringReason("PINNED"); public static final Reason DOWN_CAST = new StringReason("DOWN_CAST"); - public static final Reason INVALID_LIBRARY_SUPERTYPE = - new StringReason("INVALID_LIBRARY_SUPERTYPE"); - public static final Reason SUBTYPES = new StringReason("SUBTYPES"); public static final Reason SUBENUM_SUBTYPES = new StringReason("SUBENUM_SUBTYPES"); public static final Reason SUBENUM_INVALID_HIERARCHY = new StringReason("SUBENUM_INVALID_HIERARCHY"); public static final Reason SUBENUM_INSTANCE_FIELDS = new StringReason("SUBENUM_INSTANCE_FIELDS"); - public static final Reason SUBENUM_STATIC_MEMBER = new StringReason("SUBENUM_STATIC_MEMBER"); public static final Reason MANY_INSTANCE_FIELDS = new StringReason("MANY_INSTANCE_FIELDS"); public static final Reason DEFAULT_METHOD_INVOKE = new StringReason("DEFAULT_METHOD_INVOKE"); public static final Reason UNRESOLVABLE_FIELD = new StringReason("UNRESOLVABLE_FIELD");
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 40b2d19..e069c30 100644 --- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java +++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -2419,7 +2419,6 @@ public boolean enableSwitchToIfRewriting = true; public boolean enableEnumUnboxingDebugLogs = System.getProperty("com.android.tools.r8.enableEnumUnboxingDebugLogs") != null; - public boolean enableEnumWithSubtypesUnboxing = true; public boolean enableVerticalClassMergerLensAssertion = false; public boolean forceRedundantConstNumberRemoval = false; public boolean enableExperimentalDesugaredLibraryKeepRuleGenerator = false;
diff --git a/src/test/java/com/android/tools/r8/enumunboxing/enummerging/BasicEnumMergingKeepSubtypeTest.java b/src/test/java/com/android/tools/r8/enumunboxing/enummerging/BasicEnumMergingKeepSubtypeTest.java index 4f1d44e..c2446e9 100644 --- a/src/test/java/com/android/tools/r8/enumunboxing/enummerging/BasicEnumMergingKeepSubtypeTest.java +++ b/src/test/java/com/android/tools/r8/enumunboxing/enummerging/BasicEnumMergingKeepSubtypeTest.java
@@ -42,7 +42,6 @@ .addKeepMainRule(Main.class) .addKeepRules("-keep class " + SUBTYPE_NAME + " { public void method(); }") .addKeepRules(enumKeepRules.getKeepRules()) - .addOptionsModification(opt -> opt.testing.enableEnumWithSubtypesUnboxing = true) .addEnumUnboxingInspector( inspector -> inspector.assertNotUnboxed(EnumWithVirtualOverride.class)) .addOptionsModification(opt -> enableEnumOptions(opt, enumValueOptimization))
diff --git a/src/test/java/com/android/tools/r8/enumunboxing/enummerging/NoEnumFlagEnumMergingTest.java b/src/test/java/com/android/tools/r8/enumunboxing/enummerging/NoEnumFlagEnumMergingTest.java new file mode 100644 index 0000000..61df4af --- /dev/null +++ b/src/test/java/com/android/tools/r8/enumunboxing/enummerging/NoEnumFlagEnumMergingTest.java
@@ -0,0 +1,127 @@ +// Copyright (c) 2023, the R8 project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. +package com.android.tools.r8.enumunboxing.enummerging; + +import com.android.tools.r8.NeverInline; +import com.android.tools.r8.TestParameters; +import com.android.tools.r8.ToolHelper; +import com.android.tools.r8.enumunboxing.EnumUnboxingTestBase; +import com.android.tools.r8.graph.ClassAccessFlags; +import com.android.tools.r8.references.Reference; +import com.android.tools.r8.utils.DescriptorUtils; +import java.io.IOException; +import java.nio.file.Path; +import java.util.List; +import java.util.stream.Collectors; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameters; + +@RunWith(Parameterized.class) +public class NoEnumFlagEnumMergingTest extends EnumUnboxingTestBase { + + private final TestParameters parameters; + private final boolean enumValueOptimization; + private final EnumKeepRules enumKeepRules; + + @Parameters(name = "{0} valueOpt: {1} keep: {2}") + public static List<Object[]> data() { + return enumUnboxingTestParameters(); + } + + public NoEnumFlagEnumMergingTest( + TestParameters parameters, boolean enumValueOptimization, EnumKeepRules enumKeepRules) { + this.parameters = parameters; + this.enumValueOptimization = enumValueOptimization; + this.enumKeepRules = enumKeepRules; + } + + @Test + public void testEnumUnboxing() throws Exception { + testForR8(parameters.getBackend()) + .addProgramFiles(getEnumSubtypesOrOtherInputs(false)) + .addProgramClassFileData(getSubEnumProgramData(getEnumSubtypesOrOtherInputs(true))) + .addKeepMainRule(Main.class) + .addKeepRules(enumKeepRules.getKeepRules()) + .addEnumUnboxingInspector(inspector -> inspector.assertUnboxed(MyEnum2Cases.class)) + .enableInliningAnnotations() + .addOptionsModification(opt -> enableEnumOptions(opt, enumValueOptimization)) + .setMinApi(parameters) + .run(parameters.getRuntime(), Main.class) + .assertSuccessWithOutputLines("336", "74", "96", "44"); + } + + private List<Path> getEnumSubtypesOrOtherInputs(boolean enumSubtypes) throws IOException { + return ToolHelper.getClassFilesForInnerClasses(getClass()).stream() + .filter(path -> isMyEnum2CasesSubtype(path) == enumSubtypes) + .collect(Collectors.toList()); + } + + private boolean isMyEnum2CasesSubtype(Path c) { + return c.toString().contains("MyEnum2Cases") && !c.toString().endsWith("MyEnum2Cases.class"); + } + + private List<byte[]> getSubEnumProgramData(List<Path> input) { + // Some Kotlin enum subclasses don't have the enum flag set. See b/315186101. + return input.stream() + .map( + path -> { + try { + String subtype = path.getFileName().toString(); + String subtypeNoClass = + subtype.substring(0, (subtype.length() - ".class".length())); + String descr = + DescriptorUtils.replaceSimpleClassNameInDescriptor( + DescriptorUtils.javaTypeToDescriptor(MyEnum2Cases.class.getTypeName()), + subtypeNoClass); + return transformer(path, Reference.classFromDescriptor(descr)) + .setAccessFlags(ClassAccessFlags::unsetEnum) + .transform(); + } catch (IOException e) { + throw new RuntimeException(e); + } + }) + .collect(Collectors.toList()); + } + + enum MyEnum2Cases { + A(8) { + @NeverInline + @Override + public long operate(long another) { + return num * another; + } + }, + B(32) { + @NeverInline + @Override + public long operate(long another) { + return num + another; + } + }; + final long num; + + MyEnum2Cases(long num) { + this.num = num; + } + + public abstract long operate(long another); + } + + static class Main { + + public static void main(String[] args) { + System.out.println(MyEnum2Cases.A.operate(42)); + System.out.println(MyEnum2Cases.B.operate(42)); + System.out.println(indirect(MyEnum2Cases.A)); + System.out.println(indirect(MyEnum2Cases.B)); + } + + @NeverInline + public static long indirect(MyEnum2Cases e) { + return e.operate(12); + } + } +}