Reland "Update the type-information after checkcast removal"
This reverts commit cfb96799497ad78ca35ae0287fc5f5c1d8eed8f4.
Bug: 134546895
Bug: 133739171
Change-Id: Ic19d74de581a42d6a35dc56d7acab66684b13d62
diff --git a/src/main/java/com/android/tools/r8/ir/analysis/type/NullabilityVariants.java b/src/main/java/com/android/tools/r8/ir/analysis/type/NullabilityVariants.java
index e9baec4..9a0f95a 100644
--- a/src/main/java/com/android/tools/r8/ir/analysis/type/NullabilityVariants.java
+++ b/src/main/java/com/android/tools/r8/ir/analysis/type/NullabilityVariants.java
@@ -12,6 +12,7 @@
private T maybeNullVariant;
private T definitelyNullVariant;
private T definitelyNotNullVariant;
+ private T bottomVariant;
public static <T extends ReferenceTypeLatticeElement> T create(
Nullability nullability, Function<NullabilityVariants<T>, T> callback) {
@@ -26,9 +27,11 @@
maybeNullVariant = element;
} else if (nullability == Nullability.definitelyNull()) {
definitelyNullVariant = element;
- } else {
- assert nullability == Nullability.definitelyNotNull();
+ } else if (nullability == Nullability.definitelyNotNull()) {
definitelyNotNullVariant = element;
+ } else {
+ assert nullability == Nullability.bottom();
+ bottomVariant = element;
}
}
@@ -37,9 +40,11 @@
return maybeNullVariant;
} else if (nullability == Nullability.definitelyNull()) {
return definitelyNullVariant;
- } else {
- assert nullability == Nullability.definitelyNotNull();
+ } else if (nullability == Nullability.definitelyNotNull()) {
return definitelyNotNullVariant;
+ } else {
+ assert nullability == Nullability.bottom();
+ return bottomVariant;
}
}
diff --git a/src/main/java/com/android/tools/r8/ir/code/IRCode.java b/src/main/java/com/android/tools/r8/ir/code/IRCode.java
index e6249ff..9e045d9 100644
--- a/src/main/java/com/android/tools/r8/ir/code/IRCode.java
+++ b/src/main/java/com/android/tools/r8/ir/code/IRCode.java
@@ -11,6 +11,7 @@
import com.android.tools.r8.graph.DexType;
import com.android.tools.r8.ir.analysis.TypeChecker;
import com.android.tools.r8.ir.analysis.type.ClassTypeLatticeElement;
+import com.android.tools.r8.ir.analysis.type.Nullability;
import com.android.tools.r8.ir.analysis.type.TypeLatticeElement;
import com.android.tools.r8.ir.conversion.IRBuilder;
import com.android.tools.r8.origin.Origin;
@@ -734,6 +735,26 @@
});
}
+ public boolean verifyNoNullabilityBottomTypes() {
+ Predicate<Value> verifyValue =
+ v -> {
+ assert v.getTypeLattice().isPrimitive()
+ || v.getTypeLattice().asReferenceTypeLatticeElement().nullability()
+ != Nullability.bottom();
+ return true;
+ };
+ return verifySSATypeLattice(
+ v -> {
+ // StackValues is an artificial type created to allow returning multiple values from an
+ // instruction.
+ if (v instanceof StackValues) {
+ return Stream.of(((StackValues) v).getStackValues()).allMatch(verifyValue);
+ } else {
+ return verifyValue(v);
+ }
+ });
+ }
+
private boolean verifySSATypeLattice(Predicate<Value> tester) {
for (BasicBlock block : blocks) {
for (Instruction instruction : block.getInstructions()) {
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
index 2aea633..ec6da10 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
@@ -41,10 +41,10 @@
import com.android.tools.r8.ir.code.InvokeStatic;
import com.android.tools.r8.ir.code.NumericType;
import com.android.tools.r8.ir.code.Value;
+import com.android.tools.r8.ir.desugar.BackportedMethodRewriter;
import com.android.tools.r8.ir.desugar.CovariantReturnTypeAnnotationTransformer;
import com.android.tools.r8.ir.desugar.D8NestBasedAccessDesugaring;
import com.android.tools.r8.ir.desugar.InterfaceMethodRewriter;
-import com.android.tools.r8.ir.desugar.BackportedMethodRewriter;
import com.android.tools.r8.ir.desugar.LambdaRewriter;
import com.android.tools.r8.ir.desugar.StringConcatRewriter;
import com.android.tools.r8.ir.desugar.TwrCloseResourceRewriter;
@@ -1038,6 +1038,8 @@
// dead code which is removed right before register allocation in performRegisterAllocation.
deadCodeRemover.run(code);
assert code.isConsistentSSA();
+ // Assert that we do not have unremoved dead code in the output.
+ assert code.verifyNoNullabilityBottomTypes();
if (options.enableDesugaring && enableTryWithResourcesDesugaring()) {
codeRewriter.rewriteThrowableAddAndGetSuppressed(code);
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java b/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
index af2aeba..6576097 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
@@ -1991,14 +1991,32 @@
if (!appView.enableWholeProgramOptimizations()) {
return;
}
-
+ // If we can remove a CheckCast it is due to us having at least as much information about the
+ // type as the CheckCast gives. We then need to propagate that information to the users of
+ // the CheckCast to ensure further optimizations and removals of CheckCast:
+ //
+ // : 1: NewArrayEmpty v2 <- v1(1) java.lang.String[] <-- v2 = String[]
+ // ...
+ // : 2: CheckCast v5 <- v2; java.lang.Object[] <-- v5 = Object[]
+ // ...
+ // : 3: ArrayGet v7 <- v5, v6(0) <-- v7 = Object
+ // : 4: CheckCast v8 <- v7; java.lang.String <-- v8 = String
+ // ...
+ //
+ // When looking at line 2 we can conclude that the CheckCast is trivial because v2 is String[]
+ // and remove it. However, v7 is still only known to be Object and we cannot remove the
+ // CheckCast at line 4 unless we update v7 with the most precise information by narrowing the
+ // affected values of v5. We therefore have to run the type analysis after each CheckCast
+ // removal.
+ TypeAnalysis typeAnalysis = new TypeAnalysis(appView, code.method);
+ Set<Value> affectedValues = Sets.newIdentityHashSet();
InstructionIterator it = code.instructionIterator();
boolean needToRemoveTrivialPhis = false;
while (it.hasNext()) {
Instruction current = it.next();
if (current.isCheckCast()) {
boolean hasPhiUsers = current.outValue().numberOfPhiUsers() != 0;
- if (removeCheckCastInstructionIfTrivial(current.asCheckCast(), it, code)) {
+ if (removeCheckCastInstructionIfTrivial(current.asCheckCast(), it, code, affectedValues)) {
needToRemoveTrivialPhis |= hasPhiUsers;
}
} else if (current.isInstanceOf()) {
@@ -2007,6 +2025,10 @@
needToRemoveTrivialPhis |= hasPhiUsers;
}
}
+ if (!affectedValues.isEmpty()) {
+ typeAnalysis.narrowing(affectedValues);
+ affectedValues.clear();
+ }
}
// ... v1
// ...
@@ -2022,7 +2044,7 @@
// Returns true if the given check-cast instruction was removed.
private boolean removeCheckCastInstructionIfTrivial(
- CheckCast checkCast, InstructionIterator it, IRCode code) {
+ CheckCast checkCast, InstructionIterator it, IRCode code, Set<Value> affectedValues) {
Value inValue = checkCast.object();
Value outValue = checkCast.outValue();
DexType castType = checkCast.getType();
@@ -2054,7 +2076,15 @@
&& outValue.isUsed()
&& outValue.numberOfPhiUsers() == 0
&& outValue.uniqueUsers().stream().allMatch(isCheckcastToSubtype)) {
+ // The removeOrReplaceByDebugLocalWrite will propagate the incoming value for the CheckCast
+ // to the users of the CheckCast's out value.
+ //
+ // v2 = CheckCast A v1 ~~> DebugLocalWrite $v0 <- v1
+ //
+ // The DebugLocalWrite is not a user of the outvalue, we therefore have to wait and take the
+ // CheckCast invalue users that includes the potential DebugLocalWrite.
removeOrReplaceByDebugLocalWrite(checkCast, it, inValue, outValue);
+ affectedValues.addAll(inValue.affectedValues());
return true;
}
@@ -2074,7 +2104,15 @@
// A a = ...
// B b = (B) a;
assert inTypeLattice.lessThanOrEqual(outTypeLattice, appView);
+ // The removeOrReplaceByDebugLocalWrite will propagate the incoming value for the CheckCast
+ // to the users of the CheckCast's out value.
+ //
+ // v2 = CheckCast A v1 ~~> DebugLocalWrite $v0 <- v1
+ //
+ // The DebugLocalWrite is not a user of the outvalue, we therefore have to wait and take the
+ // CheckCast invalue users that includes the potential DebugLocalWrite.
removeOrReplaceByDebugLocalWrite(checkCast, it, inValue, outValue);
+ affectedValues.addAll(inValue.affectedValues());
return true;
}
@@ -2165,13 +2203,14 @@
}
}
if (result != InstanceOfResult.UNKNOWN) {
- it.replaceCurrentInstruction(
+ ConstNumber newInstruction =
new ConstNumber(
new Value(
code.valueNumberGenerator.next(),
TypeLatticeElement.INT,
instanceOf.outValue().getLocalInfo()),
- result == InstanceOfResult.TRUE ? 1 : 0));
+ result == InstanceOfResult.TRUE ? 1 : 0);
+ it.replaceCurrentInstruction(newInstruction);
return true;
}
return false;
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/checkcast/CheckCastDebugTestRunner.java b/src/test/java/com/android/tools/r8/ir/optimize/checkcast/CheckCastDebugTestRunner.java
index e440876..52a1f72 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/checkcast/CheckCastDebugTestRunner.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/checkcast/CheckCastDebugTestRunner.java
@@ -81,7 +81,7 @@
assertThat(method, isPresent());
long count =
Streams.stream(method.iterateInstructions(InstructionSubject::isCheckCast)).count();
- assertEquals(1, count);
+ assertEquals(0, count);
DebugTestConfig config = backend == Backend.CF
? new CfDebugTestConfig()
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/checkcast/CheckCastRemovalTest.java b/src/test/java/com/android/tools/r8/ir/optimize/checkcast/CheckCastRemovalTest.java
index 83890de..aa58df7 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/checkcast/CheckCastRemovalTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/checkcast/CheckCastRemovalTest.java
@@ -218,7 +218,6 @@
}
@Test
- @Ignore("b/133739171")
public void bothUpAndDowncast() throws Exception {
JasminBuilder builder = new JasminBuilder();
ClassBuilder classBuilder = builder.addClass(CLASS_NAME);
