Ensure value materializable in trivial phi removal
In the concrete issue reported the single field value that a phi resolved
to was not accessible in the context of the phi.
Bug: b/345248270
Change-Id: I41527a6df72726135c5c35fa9b4a8433f377f8e5
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/phis/EffectivelyTrivialPhiOptimization.java b/src/main/java/com/android/tools/r8/ir/optimize/phis/EffectivelyTrivialPhiOptimization.java
index bf3bc86..514ed67 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/phis/EffectivelyTrivialPhiOptimization.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/phis/EffectivelyTrivialPhiOptimization.java
@@ -182,7 +182,10 @@
return new SingleValueOrValue(worklist.getSeenSet());
}
if (foundDifferentOperandValuesWithSameAbstractValue) {
- if (representativeOperandAbstractValue.isSingleValue()) {
+ if (representativeOperandAbstractValue.isSingleValue()
+ && representativeOperandAbstractValue
+ .asSingleValue()
+ .isMaterializableInContext(appView, code.context())) {
return new SingleValueOrValue(
worklist.getSeenSet(), representativeOperandAbstractValue.asSingleValue());
}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/effectivelytrivialphioptimization/Regress345248270ConstClassTest.java b/src/test/java/com/android/tools/r8/ir/optimize/effectivelytrivialphioptimization/Regress345248270ConstClassTest.java
index dcd81ac..684f3a4 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/effectivelytrivialphioptimization/Regress345248270ConstClassTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/effectivelytrivialphioptimization/Regress345248270ConstClassTest.java
@@ -3,10 +3,6 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8.ir.optimize.effectivelytrivialphioptimization;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
-
-import com.android.tools.r8.CompilationFailedException;
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
@@ -30,7 +26,7 @@
return getTestParameters().withAllRuntimesAndApiLevels().build();
}
- private static final String EXPECTED_OUTPUT = StringUtils.lines("Hello, world!");
+ private static final String EXPECTED_OUTPUT = StringUtils.lines("true");
@Test
public void testD8() throws Exception {
@@ -46,24 +42,16 @@
@Test
public void testR8() throws Exception {
- try {
- testForR8(parameters.getBackend())
- .addInnerClasses(getClass())
- .addProgramClasses(
- I.class, PublicAccessor.class, PublicAccessor.getPackagePrivateImplementationClass())
- .addKeepMainRule(TestClass.class)
- .setMinApi(parameters.getApiLevel())
- .enableNeverClassInliningAnnotations()
- .enableNoAccessModificationAnnotationsForMembers()
- .run(parameters.getRuntime(), TestClass.class)
- .assertSuccessWithOutput(EXPECTED_OUTPUT);
- fail("Expected exception");
- } catch (CompilationFailedException e) {
- // TODO(b/345248270): Should not hit an assertion.
- assertTrue(e.getCause() instanceof AssertionError);
- return;
- }
- fail("Expected CompilationFailedException");
+ testForR8(parameters.getBackend())
+ .addInnerClasses(getClass())
+ .addProgramClasses(
+ I.class, PublicAccessor.class, PublicAccessor.getPackagePrivateImplementationClass())
+ .addKeepMainRule(TestClass.class)
+ .setMinApi(parameters.getApiLevel())
+ .enableNeverClassInliningAnnotations()
+ .enableNoAccessModificationAnnotationsForMembers()
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithOutput(EXPECTED_OUTPUT);
}
static class TestClass {
@@ -78,9 +66,15 @@
return r;
}
+ public static Class<?> test2() {
+ return System.currentTimeMillis() > 0
+ ? PublicAccessor.getPackagePrivateImplementationClass()
+ : null;
+ }
+
public static void main(String[] args) {
- test();
- System.out.println("Hello, world!");
+ Class<?> c = test();
+ System.out.println(c == test2());
}
}
}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/effectivelytrivialphioptimization/Regress345248270Test.java b/src/test/java/com/android/tools/r8/ir/optimize/effectivelytrivialphioptimization/Regress345248270Test.java
index aa916ba..0b0fcb3 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/effectivelytrivialphioptimization/Regress345248270Test.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/effectivelytrivialphioptimization/Regress345248270Test.java
@@ -3,16 +3,19 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8.ir.optimize.effectivelytrivialphioptimization;
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
-import com.android.tools.r8.CompilationFailedException;
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
import com.android.tools.r8.ir.optimize.effectivelytrivialphioptimization.b345248270.I;
import com.android.tools.r8.ir.optimize.effectivelytrivialphioptimization.b345248270.PublicAccessor;
import com.android.tools.r8.utils.StringUtils;
+import com.android.tools.r8.utils.codeinspector.ClassSubject;
+import com.android.tools.r8.utils.codeinspector.InstructionSubject;
+import com.android.tools.r8.utils.codeinspector.Matchers;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@@ -30,7 +33,7 @@
return getTestParameters().withAllRuntimesAndApiLevels().build();
}
- private static final String EXPECTED_OUTPUT = StringUtils.lines("Hello, world!");
+ private static final String EXPECTED_OUTPUT = StringUtils.lines("true");
@Test
public void testD8() throws Exception {
@@ -46,24 +49,42 @@
@Test
public void testR8() throws Exception {
- try {
- testForR8(parameters.getBackend())
- .addInnerClasses(getClass())
- .addProgramClasses(
- I.class, PublicAccessor.class, PublicAccessor.getPackagePrivateImplementationClass())
- .addKeepMainRule(TestClass.class)
- .setMinApi(parameters.getApiLevel())
- .enableNeverClassInliningAnnotations()
- .enableNoAccessModificationAnnotationsForMembers()
- .run(parameters.getRuntime(), TestClass.class)
- .assertSuccessWithOutput(EXPECTED_OUTPUT);
- fail("Expected exception");
- } catch (CompilationFailedException e) {
- // TODO(b/345248270): Should not hit an assertion.
- assertTrue(e.getCause() instanceof AssertionError);
- return;
- }
- fail("Expected CompilationFailedException");
+ testForR8(parameters.getBackend())
+ .addInnerClasses(getClass())
+ .addProgramClasses(
+ I.class, PublicAccessor.class, PublicAccessor.getPackagePrivateImplementationClass())
+ .addKeepMainRule(TestClass.class)
+ .setMinApi(parameters.getApiLevel())
+ .enableNeverClassInliningAnnotations()
+ .enableNoAccessModificationAnnotationsForMembers()
+ .run(parameters.getRuntime(), TestClass.class)
+ .inspect(
+ inspector -> {
+ assertThat(inspector.clazz(TestClass.class), isPresent());
+ // test is inlined into main.
+ assertThat(
+ inspector.clazz(TestClass.class).uniqueMethodWithOriginalName("test"),
+ Matchers.isAbsent());
+ ClassSubject packagePrivateImplementation =
+ inspector.clazz(PublicAccessor.getPackagePrivateImplementationClass());
+ assertThat(packagePrivateImplementation, isPresent());
+ // No direct field access of the package private field.
+ assertTrue(
+ inspector
+ .clazz(TestClass.class)
+ .mainMethod()
+ .streamInstructions()
+ .filter(InstructionSubject::isFieldAccess)
+ .map(InstructionSubject::getField)
+ .noneMatch(
+ f ->
+ f.getType()
+ .isIdenticalTo(
+ packagePrivateImplementation
+ .getDexProgramClass()
+ .getType())));
+ })
+ .assertSuccessWithOutput(EXPECTED_OUTPUT);
}
static class TestClass {
@@ -78,9 +99,15 @@
return r;
}
+ public static I test2() {
+ return System.currentTimeMillis() > 0
+ ? PublicAccessor.getPackagePrivateImplementation()
+ : null;
+ }
+
public static void main(String[] args) {
- test();
- System.out.println("Hello, world!");
+ I i = test();
+ System.out.println(i == test2());
}
}
}
