Revert "More aggressive redundant field load elimination for static final fields"
This reverts commit e1f9e833203585965d1d216a4122cc4ee2c8a6e2.
Revert "Simplify value-may-depend-on-environment analysis"
This reverts commit 9eed74e0c1279bfa754a94e5f202e8dd77891f65.
Change-Id: I89a16536d61d6e2d275674efc052f90ae152bc98
Reason for revert: Bot failures
diff --git a/src/main/java/com/android/tools/r8/ir/analysis/ValueMayDependOnEnvironmentAnalysis.java b/src/main/java/com/android/tools/r8/ir/analysis/ValueMayDependOnEnvironmentAnalysis.java
index f417d65..05416df 100644
--- a/src/main/java/com/android/tools/r8/ir/analysis/ValueMayDependOnEnvironmentAnalysis.java
+++ b/src/main/java/com/android/tools/r8/ir/analysis/ValueMayDependOnEnvironmentAnalysis.java
@@ -20,12 +20,16 @@
import com.android.tools.r8.ir.code.NewArrayEmpty;
import com.android.tools.r8.ir.code.NewArrayFilledData;
import com.android.tools.r8.ir.code.NewInstance;
+import com.android.tools.r8.ir.code.StaticGet;
import com.android.tools.r8.ir.code.StaticPut;
import com.android.tools.r8.ir.code.Value;
import com.android.tools.r8.ir.optimize.info.initializer.InstanceInitializerInfo;
+import com.android.tools.r8.utils.ListUtils;
import com.android.tools.r8.utils.LongInterval;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.IdentityHashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
@@ -115,6 +119,9 @@
if (isNewInstanceWithoutEnvironmentDependentFields(root, assumedNotToDependOnEnvironment)) {
return false;
}
+ if (isAliasOfValueThatIsIndependentOfEnvironment(root, assumedNotToDependOnEnvironment)) {
+ return false;
+ }
return true;
} finally {
boolean changed = visited.remove(root);
@@ -407,4 +414,49 @@
return false;
}
+
+ private boolean isAliasOfValueThatIsIndependentOfEnvironment(
+ Value value, Set<Value> assumedNotToDependOnEnvironment) {
+ // If we are inside a class initializer, and we are reading a final field of the enclosing
+ // class, then check if there is a single write to that field in the class initializer, and that
+ // the value being written into that field does not depend on the environment.
+ //
+ // The reason why we do not currently treat final fields that are written in multiple places is
+ // that the value of the field could then be dependent on the environment due to the control
+ // flow.
+ if (code.method.isClassInitializer()) {
+ assert !value.hasAliasedValue();
+ if (value.isPhi()) {
+ return false;
+ }
+ Instruction definition = value.definition;
+ if (definition.isStaticGet()) {
+ StaticGet staticGet = definition.asStaticGet();
+ DexEncodedField field = appView.appInfo().resolveField(staticGet.getField());
+ if (field != null && field.holder() == context) {
+ List<StaticPut> finalFieldPuts = computeFinalFieldPuts().get(field);
+ if (finalFieldPuts == null || finalFieldPuts.size() != 1) {
+ return false;
+ }
+ StaticPut staticPut = ListUtils.first(finalFieldPuts);
+ return !valueMayDependOnEnvironment(staticPut.value(), assumedNotToDependOnEnvironment);
+ }
+ }
+ }
+ return false;
+ }
+
+ private Map<DexEncodedField, List<StaticPut>> computeFinalFieldPuts() {
+ assert code.method.isClassInitializer();
+ if (finalFieldPuts == null) {
+ finalFieldPuts = new IdentityHashMap<>();
+ for (StaticPut staticPut : code.<StaticPut>instructions(Instruction::isStaticPut)) {
+ DexEncodedField field = appView.appInfo().resolveField(staticPut.getField());
+ if (field != null && field.holder() == context && field.isFinal()) {
+ finalFieldPuts.computeIfAbsent(field, ignore -> new ArrayList<>()).add(staticPut);
+ }
+ }
+ }
+ return finalFieldPuts;
+ }
}
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/RedundantFieldLoadElimination.java b/src/main/java/com/android/tools/r8/ir/optimize/RedundantFieldLoadElimination.java
index 455d733..81b32e8 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/RedundantFieldLoadElimination.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/RedundantFieldLoadElimination.java
@@ -206,12 +206,7 @@
// A field get on a different class can cause <clinit> to run and change static
// field values.
killNonFinalActiveFields(staticGet);
- FieldValue value = new ExistingValue(staticGet.value());
- if (definition.isFinal() && definition.isStatic()) {
- activeState.putFinalStaticField(field, value);
- } else {
- activeState.putNonFinalStaticField(field, value);
- }
+ activeState.putNonFinalStaticField(field, new ExistingValue(staticGet.value()));
}
} else if (instruction.isStaticPut()) {
StaticPut staticPut = instruction.asStaticPut();
@@ -492,9 +487,9 @@
if (instruction.isInstanceGet()) {
Value object = instruction.asInstanceGet().object().getAliasedValue();
FieldAndObject fieldAndObject = new FieldAndObject(field, object);
- removeInstanceField(fieldAndObject);
+ removeNonFinalInstanceField(fieldAndObject);
} else if (instruction.isStaticGet()) {
- removeStaticField(field);
+ removeNonFinalStaticField(field);
}
}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/canonicalization/EnumCanonicalizationTest.java b/src/test/java/com/android/tools/r8/ir/optimize/canonicalization/EnumCanonicalizationTest.java
index 589fa3f..1e323fb 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/canonicalization/EnumCanonicalizationTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/canonicalization/EnumCanonicalizationTest.java
@@ -68,7 +68,8 @@
MethodSubject mainMethodSubject = classSubject.mainMethod();
assertThat(mainMethodSubject, isPresent());
assertEquals(
- 1,
+ // No canonicalization when generating class files.
+ parameters.isCfRuntime() ? 3 : 1,
mainMethodSubject
.streamInstructions()
.filter(InstructionSubject::isStaticGet)
@@ -76,7 +77,7 @@
.filter(enumFieldSubject.getField().field::equals)
.count());
assertEquals(
- 1,
+ 3,
mainMethodSubject
.streamInstructions()
.filter(InstructionSubject::isStaticGet)
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/canonicalization/StaticFinalLibraryFieldCanonicalizationTest.java b/src/test/java/com/android/tools/r8/ir/optimize/canonicalization/StaticFinalLibraryFieldCanonicalizationTest.java
index 275f0e4..7da9e81 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/canonicalization/StaticFinalLibraryFieldCanonicalizationTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/canonicalization/StaticFinalLibraryFieldCanonicalizationTest.java
@@ -66,7 +66,8 @@
MethodSubject mainMethodSubject = testClassSubject.mainMethod();
assertThat(mainMethodSubject, isPresent());
assertEquals(
- 1, mainMethodSubject.streamInstructions().filter(InstructionSubject::isStaticGet).count());
+ systemHasClassInitializationSideEffects || parameters.isCfRuntime() ? 2 : 1,
+ mainMethodSubject.streamInstructions().filter(InstructionSubject::isStaticGet).count());
}
static class TestClass {
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/redundantfieldloadelimination/RedundantFinalStaticFieldLoadAfterStoreTest.java b/src/test/java/com/android/tools/r8/ir/optimize/redundantfieldloadelimination/RedundantFinalStaticFieldLoadAfterStoreTest.java
index 2b910f5..fe3d79a 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/redundantfieldloadelimination/RedundantFinalStaticFieldLoadAfterStoreTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/redundantfieldloadelimination/RedundantFinalStaticFieldLoadAfterStoreTest.java
@@ -65,8 +65,9 @@
MethodSubject mMethodSubject = aClassSubject.uniqueMethodWithName("m");
assertThat(mMethodSubject, isPresent());
+ // TODO(b/152196923): Should be 0.
assertEquals(
- 1,
+ 2,
countStaticGetInstructions(
mMethodSubject.asFoundMethodSubject(), fFieldSubject.asFoundFieldSubject()));
}
