Do not share constant use one time by a phi
- Do no share constant used only one time by a phi instruction,
on contrary move it directly to the branch corresponding to
the phi operand. It will avoid to define constant on useless path.
- It save about 11k of dex code for deploy.jar of GMSCore V9 and V10.
Bug: 65432240
Change-Id: I0b4b9d57e083cbcd5e0e05c696f3d1938d092561
diff --git a/src/main/java/com/android/tools/r8/ir/code/Phi.java b/src/main/java/com/android/tools/r8/ir/code/Phi.java
index 28a4d74..a644470 100644
--- a/src/main/java/com/android/tools/r8/ir/code/Phi.java
+++ b/src/main/java/com/android/tools/r8/ir/code/Phi.java
@@ -363,4 +363,8 @@
public Set<Value> getDebugValues() {
return debugValues != null ? debugValues : ImmutableSet.of();
}
+
+ public boolean usesValueOneTime(Value usedValue) {
+ return operands.indexOf(usedValue) == operands.lastIndexOf(usedValue);
+ }
}
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 fbde02b..5570abe 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
@@ -1177,7 +1177,16 @@
// needs to go in the immediate dominator block so that it is available for phi moves.
for (Phi phi : instruction.outValue().uniquePhiUsers()) {
if (phi.getBlock() == dominator) {
- dominator = dominatorTree.immediateDominator(dominator);
+ if (instruction.outValue().numberOfAllUsers() == 1 &&
+ phi.usesValueOneTime(instruction.outValue())) {
+ // Out value is used only one time, move the constant directly to the corresponding
+ // branch rather than into the dominator to avoid to generate a const on paths which
+ // does not required it.
+ int predIndex = phi.getOperands().indexOf(instruction.outValue());
+ dominator = dominator.getPredecessors().get(predIndex);
+ } else {
+ dominator = dominatorTree.immediateDominator(dominator);
+ }
break;
}
}
diff --git a/src/test/java/com/android/tools/r8/jasmin/JasminTestBase.java b/src/test/java/com/android/tools/r8/jasmin/JasminTestBase.java
index 1573b34..92a755e 100644
--- a/src/test/java/com/android/tools/r8/jasmin/JasminTestBase.java
+++ b/src/test/java/com/android/tools/r8/jasmin/JasminTestBase.java
@@ -4,21 +4,29 @@
package com.android.tools.r8.jasmin;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
import com.android.tools.r8.CompilationException;
import com.android.tools.r8.R8;
import com.android.tools.r8.ToolHelper;
import com.android.tools.r8.ToolHelper.ProcessResult;
+import com.android.tools.r8.dex.ApplicationReader;
+import com.android.tools.r8.errors.DexOverflowException;
import com.android.tools.r8.graph.AppInfo;
import com.android.tools.r8.graph.AppInfoWithSubtyping;
import com.android.tools.r8.graph.DexApplication;
+import com.android.tools.r8.graph.DexEncodedMethod;
import com.android.tools.r8.jasmin.JasminBuilder.ClassBuilder;
+import com.android.tools.r8.naming.MemberNaming.MethodSignature;
import com.android.tools.r8.naming.NamingLens;
-import com.android.tools.r8.shaking.ProguardRuleParserException;
import com.android.tools.r8.utils.AndroidApp;
+import com.android.tools.r8.utils.DexInspector;
+import com.android.tools.r8.utils.DexInspector.ClassSubject;
+import com.android.tools.r8.utils.DexInspector.MethodSubject;
import com.android.tools.r8.utils.InternalOptions;
import com.android.tools.r8.utils.OutputMode;
import com.android.tools.r8.utils.StringUtils;
+import com.android.tools.r8.utils.Timing;
import com.google.common.collect.ImmutableList;
import jasmin.ClassFile;
import java.io.File;
@@ -28,6 +36,7 @@
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.List;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Executors;
@@ -135,4 +144,65 @@
throws IOException, CompilationException, ExecutionException {
return ToolHelper.optimizeWithR8(app, new AppInfoWithSubtyping(app), options);
}
+
+ protected DexApplication buildApplication(JasminBuilder builder, InternalOptions options) {
+ try {
+ return buildApplication(AndroidApp.fromClassProgramData(builder.buildClasses()), options);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ protected DexApplication buildApplication(AndroidApp input, InternalOptions options) {
+ try {
+ options.itemFactory.resetSortedIndices();
+ return new ApplicationReader(input, options, new Timing("JasminTest")).read();
+ } catch (IOException | ExecutionException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ public String runArt(DexApplication application, InternalOptions options, String mainClass)
+ throws DexOverflowException {
+ try {
+ AndroidApp app = writeDex(application, options);
+ return runOnArt(app, mainClass);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ public AndroidApp writeDex(DexApplication application, InternalOptions options)
+ throws DexOverflowException {
+ AppInfo appInfo = new AppInfo(application);
+ try {
+ return R8.writeApplication(
+ Executors.newSingleThreadExecutor(),
+ application,
+ appInfo,
+ null,
+ NamingLens.getIdentityLens(),
+ null,
+ null,
+ options);
+ } catch (ExecutionException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ protected DexEncodedMethod getMethod(DexApplication application, String clazz,
+ MethodSignature signature) {
+ return getMethod(application,
+ clazz, signature.type, signature.name, signature.parameters);
+ }
+
+ protected DexEncodedMethod getMethod(DexApplication application, String className,
+ String returnType, String methodName, String[] parameters) {
+ DexInspector inspector = new DexInspector(application);
+ ClassSubject clazz = inspector.clazz(className);
+ assertTrue(clazz.isPresent());
+ MethodSubject method = clazz.method(returnType, methodName, Arrays.asList(parameters));
+ assertTrue(method.isPresent());
+ return method.getMethod();
+ }
}
diff --git a/src/test/java/com/android/tools/r8/jasmin/Regress65432240.java b/src/test/java/com/android/tools/r8/jasmin/Regress65432240.java
new file mode 100644
index 0000000..e8efd38
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/jasmin/Regress65432240.java
@@ -0,0 +1,72 @@
+// Copyright (c) 2017, 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.jasmin;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+import com.android.tools.r8.code.Const4;
+import com.android.tools.r8.code.IfNez;
+import com.android.tools.r8.graph.DexApplication;
+import com.android.tools.r8.graph.DexCode;
+import com.android.tools.r8.graph.DexEncodedMethod;
+import com.android.tools.r8.ir.code.ConstNumber;
+import com.android.tools.r8.naming.MemberNaming.MethodSignature;
+import com.android.tools.r8.utils.InternalOptions;
+import com.google.common.collect.ImmutableList;
+import org.junit.Test;
+
+public class Regress65432240 extends JasminTestBase {
+
+ @Test
+ public void testConstantNotIntoEntryBlock() throws Exception {
+ JasminBuilder builder = new JasminBuilder();
+ JasminBuilder.ClassBuilder clazz = builder.addClass("Test");
+
+ MethodSignature signature = clazz.addStaticMethod("test1", ImmutableList.of("I"), "I",
+ ".limit stack 3",
+ ".limit locals 2",
+ " iload 0",
+ " ifne L2",
+ "L1:",
+ " iconst_0",
+ " ireturn",
+ "L2:",
+ " iload 0",
+ " iload 0",
+ " iconst_1",
+ " isub",
+ " invokestatic Test/test1(I)I",
+ " iadd",
+ " ireturn");
+
+ clazz.addMainMethod(
+ ".limit stack 2",
+ ".limit locals 1",
+ " getstatic java/lang/System/out Ljava/io/PrintStream;",
+ " iconst_0",
+ " invokestatic Test/test1(I)I",
+ " invokestatic java/lang/Integer/toString(I)Ljava/lang/String;",
+ " invokevirtual java/io/PrintStream/print(Ljava/lang/String;)V",
+ " getstatic java/lang/System/out Ljava/io/PrintStream;",
+ " iconst_0",
+ " invokestatic Test/test1(I)I",
+ " invokestatic java/lang/Integer/toString(I)Ljava/lang/String;",
+ " invokevirtual java/io/PrintStream/print(Ljava/lang/String;)V",
+ " return");
+
+ String expected = runOnJava(builder, clazz.name);
+
+ InternalOptions options = new InternalOptions();
+ DexApplication originalApplication = buildApplication(builder, options);
+ DexApplication processedApplication = process(originalApplication, options);
+
+ DexEncodedMethod method = getMethod(processedApplication, clazz.name, signature);
+ DexCode code = method.getCode().asDexCode();
+ assertTrue(code.instructions[0] instanceof IfNez);
+
+ String artResult = runArt(processedApplication, options, clazz.name);
+ assertEquals(expected, artResult);
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/smali/SwitchRewritingTest.java b/src/test/java/com/android/tools/r8/smali/SwitchRewritingTest.java
index 3750a86..21d3b9e 100644
--- a/src/test/java/com/android/tools/r8/smali/SwitchRewritingTest.java
+++ b/src/test/java/com/android/tools/r8/smali/SwitchRewritingTest.java
@@ -302,11 +302,10 @@
DexCode code = method.getCode().asDexCode();
if (key == 0) {
assertEquals(5, code.instructions.length);
- assertTrue(code.instructions[2] instanceof IfEqz);
+ assertTrue(code.instructions[0] instanceof IfEqz);
} else {
assertEquals(6, code.instructions.length);
- assertTrue(some16BitConst(code.instructions[2]));
- assertTrue(code.instructions[3] instanceof IfEq);
+ assertTrue(code.instructions[1] instanceof IfEq);
}
}
@@ -360,13 +359,13 @@
DexEncodedMethod method = getMethod(app, signature);
DexCode code = method.getCode().asDexCode();
if (twoCaseWillUsePackedSwitch(key1, key2)) {
- assertTrue(code.instructions[3] instanceof PackedSwitch);
+ assertTrue(code.instructions[0] instanceof PackedSwitch);
} else {
if (key1 == 0) {
- assertTrue(code.instructions[3] instanceof IfEqz);
+ assertTrue(code.instructions[0] instanceof IfEqz);
} else {
// Const instruction before if.
- assertTrue(code.instructions[4] instanceof IfEq);
+ assertTrue(code.instructions[1] instanceof IfEq);
}
}
}
