Merge "Don't compare positions for non-throwing instructions in release mode."
diff --git a/src/main/java/com/android/tools/r8/graph/DexCode.java b/src/main/java/com/android/tools/r8/graph/DexCode.java
index 4b52a9e..0da9ead 100644
--- a/src/main/java/com/android/tools/r8/graph/DexCode.java
+++ b/src/main/java/com/android/tools/r8/graph/DexCode.java
@@ -95,17 +95,6 @@
this.debugInfo = debugInfo;
}
- public boolean hasDebugPositions() {
- if (debugInfo != null) {
- for (DexDebugEvent event : debugInfo.events) {
- if (event instanceof DexDebugEvent.Default) {
- return true;
- }
- }
- }
- return false;
- }
-
public DexDebugInfo debugInfoWithAdditionalFirstParameter(DexString name) {
if (debugInfo == null) {
return null;
diff --git a/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java b/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java
index f2cc53f..22a771b 100644
--- a/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java
+++ b/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java
@@ -401,12 +401,6 @@
}
}
- public boolean hasDebugPositions() {
- checkIfObsolete();
- assert code != null && code.isDexCode();
- return code.asDexCode().hasDebugPositions();
- }
-
public int getClassFileVersion() {
checkIfObsolete();
assert classFileVersion >= 0;
diff --git a/src/main/java/com/android/tools/r8/ir/code/Instruction.java b/src/main/java/com/android/tools/r8/ir/code/Instruction.java
index d3355b4..44a7a8e 100644
--- a/src/main/java/com/android/tools/r8/ir/code/Instruction.java
+++ b/src/main/java/com/android/tools/r8/ir/code/Instruction.java
@@ -406,7 +406,13 @@
if (other.getClass() != getClass()) {
return false;
}
- if (!identicalNonValueParts(other)) {
+ // In debug mode or if the instruction can throw we must account for positions, in release mode
+ // we do want to share non-throwing instructions even if their positions differ.
+ if (instructionTypeCanThrow() || allocator.getOptions().debug) {
+ if (!identicalNonValueParts(other)) {
+ return false;
+ }
+ } else if (!identicalNonValueNonPositionParts(other)) {
return false;
}
if (isInvokeDirect() && !asInvokeDirect().sameConstructorReceiverValue(other.asInvoke())) {
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/PeepholeOptimizer.java b/src/main/java/com/android/tools/r8/ir/optimize/PeepholeOptimizer.java
index eb140c4..e6791f7 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/PeepholeOptimizer.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/PeepholeOptimizer.java
@@ -253,7 +253,8 @@
changed = true;
int otherPredIndex = blockToIndex.get(wrapper);
BasicBlock otherPred = block.getPredecessors().get(otherPredIndex);
- assert Objects.equals(pred.getPosition(), otherPred.getPosition());
+ assert !allocator.getOptions().debug
+ || Objects.equals(pred.getPosition(), otherPred.getPosition());
pred.clearCatchHandlers();
pred.getInstructions().clear();
equivalence.clearComputedHash(pred);
diff --git a/src/test/java/com/android/tools/r8/release/ShareCommonCodeOnDistinctPositionsTest.java b/src/test/java/com/android/tools/r8/release/ShareCommonCodeOnDistinctPositionsTest.java
new file mode 100644
index 0000000..353952b
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/release/ShareCommonCodeOnDistinctPositionsTest.java
@@ -0,0 +1,18 @@
+// Copyright (c) 2018, 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.release;
+
+public class ShareCommonCodeOnDistinctPositionsTest {
+
+ public static void main(String[] args) {
+ int x;
+ int len = args.length;
+ if (len > 42) {
+ x = (len - 2) + len * 2;
+ } else {
+ x = (len - 2) + len * 2;
+ }
+ System.out.println(x);
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/release/ShareCommonCodeOnDistinctPositionsTestRunner.java b/src/test/java/com/android/tools/r8/release/ShareCommonCodeOnDistinctPositionsTestRunner.java
new file mode 100644
index 0000000..6bfd3d1
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/release/ShareCommonCodeOnDistinctPositionsTestRunner.java
@@ -0,0 +1,72 @@
+// Copyright (c) 2018, 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.release;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+
+import com.android.tools.r8.CompilationFailedException;
+import com.android.tools.r8.R8Command;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.ToolHelper;
+import com.android.tools.r8.utils.AndroidAppConsumers;
+import com.android.tools.r8.utils.InternalOptions.LineNumberOptimization;
+import com.android.tools.r8.utils.codeinspector.CodeInspector;
+import com.android.tools.r8.utils.codeinspector.InstructionSubject;
+import com.android.tools.r8.utils.codeinspector.LineNumberTable;
+import com.android.tools.r8.utils.codeinspector.MethodSubject;
+import com.google.common.collect.Streams;
+import it.unimi.dsi.fastutil.ints.IntCollection;
+import java.io.IOException;
+import java.util.concurrent.ExecutionException;
+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 ShareCommonCodeOnDistinctPositionsTestRunner extends TestBase {
+
+ private static final Class CLASS = ShareCommonCodeOnDistinctPositionsTest.class;
+
+ @Parameters
+ public static Backend[] parameters() {
+ return Backend.values();
+ }
+
+ private final Backend backend;
+
+ public ShareCommonCodeOnDistinctPositionsTestRunner(Backend backend) {
+ this.backend = backend;
+ }
+
+ @Test
+ public void test() throws CompilationFailedException, IOException, ExecutionException {
+ AndroidAppConsumers sink = new AndroidAppConsumers();
+ ToolHelper.runR8(
+ R8Command.builder()
+ .addLibraryFiles(runtimeJar(backend))
+ .addProgramFiles(ToolHelper.getClassFileForTestClass(CLASS))
+ .setProgramConsumer(sink.wrapProgramConsumer(emptyConsumer(backend)))
+ .setDisableMinification(true)
+ .setDisableTreeShaking(true)
+ .build(),
+ options -> options.lineNumberOptimization = LineNumberOptimization.OFF);
+ CodeInspector inspector = new CodeInspector(sink.build());
+ MethodSubject method = inspector.clazz(CLASS).mainMethod();
+ // Check that the two shared lines are not in the output (they have no throwing instructions).
+ LineNumberTable lineNumberTable = method.getLineNumberTable();
+ IntCollection lines = lineNumberTable.getLines();
+ assertFalse(lines.contains(12));
+ assertFalse(lines.contains(14));
+ // Check that the two lines have been shared, e.g., there may be only one multiplication left.
+ assertEquals(
+ "Expected only one multiplcation due to instruction sharing.",
+ // TODO(b/117539423): Implement support for sharing optimizations in the CF backend.
+ backend == Backend.DEX ? 1 : 2,
+ Streams.stream(method.iterateInstructions())
+ .filter(InstructionSubject::isMultiplication)
+ .count());
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/utils/codeinspector/AbsentMethodSubject.java b/src/test/java/com/android/tools/r8/utils/codeinspector/AbsentMethodSubject.java
index adee8cf..15e8803 100644
--- a/src/test/java/com/android/tools/r8/utils/codeinspector/AbsentMethodSubject.java
+++ b/src/test/java/com/android/tools/r8/utils/codeinspector/AbsentMethodSubject.java
@@ -80,8 +80,8 @@
}
@Override
- public boolean hasLineNumberTable() {
- return false;
+ public LineNumberTable getLineNumberTable() {
+ return null;
}
@Override
diff --git a/src/test/java/com/android/tools/r8/utils/codeinspector/CfInstructionSubject.java b/src/test/java/com/android/tools/r8/utils/codeinspector/CfInstructionSubject.java
index dedb839..067280a 100644
--- a/src/test/java/com/android/tools/r8/utils/codeinspector/CfInstructionSubject.java
+++ b/src/test/java/com/android/tools/r8/utils/codeinspector/CfInstructionSubject.java
@@ -4,6 +4,7 @@
package com.android.tools.r8.utils.codeinspector;
+import com.android.tools.r8.cf.code.CfArithmeticBinop;
import com.android.tools.r8.cf.code.CfCheckCast;
import com.android.tools.r8.cf.code.CfConstNull;
import com.android.tools.r8.cf.code.CfConstString;
@@ -212,4 +213,13 @@
public boolean isLoad() {
return instruction instanceof CfLoad;
}
+
+ @Override
+ public boolean isMultiplication() {
+ if (!(instruction instanceof CfArithmeticBinop)) {
+ return false;
+ }
+ int opcode = ((CfArithmeticBinop) instruction).getAsmOpcode();
+ return Opcodes.IMUL <= opcode && opcode <= Opcodes.DMUL;
+ }
}
diff --git a/src/test/java/com/android/tools/r8/utils/codeinspector/DexInstructionSubject.java b/src/test/java/com/android/tools/r8/utils/codeinspector/DexInstructionSubject.java
index e1d74c1..ca9ce6a 100644
--- a/src/test/java/com/android/tools/r8/utils/codeinspector/DexInstructionSubject.java
+++ b/src/test/java/com/android/tools/r8/utils/codeinspector/DexInstructionSubject.java
@@ -46,6 +46,16 @@
import com.android.tools.r8.code.IputObject;
import com.android.tools.r8.code.IputShort;
import com.android.tools.r8.code.IputWide;
+import com.android.tools.r8.code.MulDouble;
+import com.android.tools.r8.code.MulDouble2Addr;
+import com.android.tools.r8.code.MulFloat;
+import com.android.tools.r8.code.MulFloat2Addr;
+import com.android.tools.r8.code.MulInt;
+import com.android.tools.r8.code.MulInt2Addr;
+import com.android.tools.r8.code.MulIntLit16;
+import com.android.tools.r8.code.MulIntLit8;
+import com.android.tools.r8.code.MulLong;
+import com.android.tools.r8.code.MulLong2Addr;
import com.android.tools.r8.code.NewInstance;
import com.android.tools.r8.code.Nop;
import com.android.tools.r8.code.PackedSwitch;
@@ -265,4 +275,18 @@
public boolean isSparseSwitch() {
return instruction instanceof SparseSwitch;
}
+
+ @Override
+ public boolean isMultiplication() {
+ return instruction instanceof MulInt
+ || instruction instanceof MulIntLit8
+ || instruction instanceof MulIntLit16
+ || instruction instanceof MulInt2Addr
+ || instruction instanceof MulFloat
+ || instruction instanceof MulFloat2Addr
+ || instruction instanceof MulLong
+ || instruction instanceof MulLong2Addr
+ || instruction instanceof MulDouble
+ || instruction instanceof MulDouble2Addr;
+ }
}
diff --git a/src/test/java/com/android/tools/r8/utils/codeinspector/FoundMethodSubject.java b/src/test/java/com/android/tools/r8/utils/codeinspector/FoundMethodSubject.java
index f205594..f4f1660 100644
--- a/src/test/java/com/android/tools/r8/utils/codeinspector/FoundMethodSubject.java
+++ b/src/test/java/com/android/tools/r8/utils/codeinspector/FoundMethodSubject.java
@@ -6,20 +6,26 @@
import com.android.tools.r8.cf.code.CfInstruction;
import com.android.tools.r8.cf.code.CfPosition;
+import com.android.tools.r8.code.Instruction;
+import com.android.tools.r8.errors.Unimplemented;
import com.android.tools.r8.errors.Unreachable;
+import com.android.tools.r8.graph.CfCode;
import com.android.tools.r8.graph.Code;
import com.android.tools.r8.graph.DexCode;
import com.android.tools.r8.graph.DexDebugEvent;
+import com.android.tools.r8.graph.DexDebugInfo;
+import com.android.tools.r8.graph.DexDebugPositionState;
import com.android.tools.r8.graph.DexEncodedMethod;
import com.android.tools.r8.graph.DexString;
+import com.android.tools.r8.graph.JarCode;
import com.android.tools.r8.naming.MemberNaming;
import com.android.tools.r8.naming.MemberNaming.MethodSignature;
import com.android.tools.r8.naming.signature.GenericSignatureParser;
+import it.unimi.dsi.fastutil.objects.Reference2IntMap;
+import it.unimi.dsi.fastutil.objects.Reference2IntOpenHashMap;
+import java.util.Arrays;
import java.util.Iterator;
-import java.util.ListIterator;
import java.util.function.Predicate;
-import org.objectweb.asm.tree.AbstractInsnNode;
-import org.objectweb.asm.tree.LineNumberNode;
public class FoundMethodSubject extends MethodSubject {
@@ -144,40 +150,6 @@
}
@Override
- public boolean hasLineNumberTable() {
- Code code = getMethod().getCode();
- if (code.isDexCode()) {
- DexCode dexCode = code.asDexCode();
- if (dexCode.getDebugInfo() != null) {
- for (DexDebugEvent event : dexCode.getDebugInfo().events) {
- if (event instanceof DexDebugEvent.Default) {
- return true;
- }
- }
- }
- return false;
- }
- if (code.isCfCode()) {
- for (CfInstruction insn : code.asCfCode().getInstructions()) {
- if (insn instanceof CfPosition) {
- return true;
- }
- }
- return false;
- }
- if (code.isJarCode()) {
- ListIterator<AbstractInsnNode> it = code.asJarCode().getNode().instructions.iterator();
- while (it.hasNext()) {
- if (it.next() instanceof LineNumberNode) {
- return true;
- }
- }
- return false;
- }
- throw new Unreachable("Unexpected code type: " + code.getClass().getSimpleName());
- }
-
- @Override
public boolean hasLocalVariableTable() {
Code code = getMethod().getCode();
if (code.isDexCode()) {
@@ -207,6 +179,60 @@
}
@Override
+ public LineNumberTable getLineNumberTable() {
+ Code code = getMethod().getCode();
+ if (code.isDexCode()) {
+ return getDexLineNumberTable(code.asDexCode());
+ }
+ if (code.isCfCode()) {
+ return getCfLineNumberTable(code.asCfCode());
+ }
+ if (code.isJarCode()) {
+ return getJarLineNumberTable(code.asJarCode());
+ }
+ throw new Unreachable("Unexpected code type: " + code.getClass().getSimpleName());
+ }
+
+ private LineNumberTable getJarLineNumberTable(JarCode code) {
+ throw new Unimplemented("No support for inspecting the line number table for JarCode");
+ }
+
+ private LineNumberTable getCfLineNumberTable(CfCode code) {
+ int currentLine = -1;
+ Reference2IntMap<InstructionSubject> lineNumberTable =
+ new Reference2IntOpenHashMap<>(code.getInstructions().size());
+ for (CfInstruction insn : code.getInstructions()) {
+ if (insn instanceof CfPosition) {
+ currentLine = ((CfPosition) insn).getPosition().line;
+ }
+ if (currentLine != -1) {
+ lineNumberTable.put(new CfInstructionSubject(insn), currentLine);
+ }
+ }
+ return currentLine == -1 ? null : new LineNumberTable(lineNumberTable);
+ }
+
+ private LineNumberTable getDexLineNumberTable(DexCode code) {
+ DexDebugInfo debugInfo = code.getDebugInfo();
+ if (debugInfo == null) {
+ return null;
+ }
+ Reference2IntMap<InstructionSubject> lineNumberTable =
+ new Reference2IntOpenHashMap<>(code.instructions.length);
+ DexDebugPositionState state =
+ new DexDebugPositionState(debugInfo.startLine, getMethod().method);
+ Iterator<DexDebugEvent> iterator = Arrays.asList(debugInfo.events).iterator();
+ for (Instruction insn : code.instructions) {
+ int offset = insn.getOffset();
+ while (state.getCurrentPc() < offset && iterator.hasNext()) {
+ iterator.next().accept(state);
+ }
+ lineNumberTable.put(new DexInstructionSubject(insn), state.getCurrentLine());
+ }
+ return new LineNumberTable(lineNumberTable);
+ }
+
+ @Override
public String toString() {
return dexMethod.toSourceString();
}
diff --git a/src/test/java/com/android/tools/r8/utils/codeinspector/InstructionSubject.java b/src/test/java/com/android/tools/r8/utils/codeinspector/InstructionSubject.java
index fb0e0d3..6890684 100644
--- a/src/test/java/com/android/tools/r8/utils/codeinspector/InstructionSubject.java
+++ b/src/test/java/com/android/tools/r8/utils/codeinspector/InstructionSubject.java
@@ -65,4 +65,6 @@
boolean isPackedSwitch();
boolean isSparseSwitch();
+
+ boolean isMultiplication();
}
diff --git a/src/test/java/com/android/tools/r8/utils/codeinspector/LineNumberTable.java b/src/test/java/com/android/tools/r8/utils/codeinspector/LineNumberTable.java
new file mode 100644
index 0000000..7c9c30f
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/utils/codeinspector/LineNumberTable.java
@@ -0,0 +1,19 @@
+// Copyright (c) 2018, 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.utils.codeinspector;
+
+import it.unimi.dsi.fastutil.ints.IntCollection;
+import it.unimi.dsi.fastutil.objects.Reference2IntMap;
+
+public class LineNumberTable {
+ private final Reference2IntMap<InstructionSubject> lineNumberTable;
+
+ public LineNumberTable(Reference2IntMap<InstructionSubject> lineNumberTable) {
+ this.lineNumberTable = lineNumberTable;
+ }
+
+ public IntCollection getLines() {
+ return lineNumberTable.values();
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/utils/codeinspector/MethodSubject.java b/src/test/java/com/android/tools/r8/utils/codeinspector/MethodSubject.java
index 55030ca..1e21552 100644
--- a/src/test/java/com/android/tools/r8/utils/codeinspector/MethodSubject.java
+++ b/src/test/java/com/android/tools/r8/utils/codeinspector/MethodSubject.java
@@ -33,7 +33,11 @@
return null;
}
- public abstract boolean hasLineNumberTable();
+ public boolean hasLineNumberTable() {
+ return getLineNumberTable() != null;
+ }
+
+ public abstract LineNumberTable getLineNumberTable();
public abstract boolean hasLocalVariableTable();
}
