Don't simplify branches with observable line changes
Bug: b/339064674
Change-Id: I92c40837b6625a0a27b48b07b14de9fb94b8541b
diff --git a/src/main/java/com/android/tools/r8/ir/analysis/equivalence/BasicBlockBehavioralSubsumption.java b/src/main/java/com/android/tools/r8/ir/analysis/equivalence/BasicBlockBehavioralSubsumption.java
index c37be79..35619e1 100644
--- a/src/main/java/com/android/tools/r8/ir/analysis/equivalence/BasicBlockBehavioralSubsumption.java
+++ b/src/main/java/com/android/tools/r8/ir/analysis/equivalence/BasicBlockBehavioralSubsumption.java
@@ -18,6 +18,7 @@
import com.android.tools.r8.ir.code.InstructionIterator;
import com.android.tools.r8.ir.code.InvokeDirect;
import com.android.tools.r8.ir.code.Phi;
+import com.android.tools.r8.ir.code.Position;
import com.android.tools.r8.ir.code.Return;
import com.android.tools.r8.ir.code.Value;
import com.android.tools.r8.utils.SetUtils;
@@ -43,8 +44,10 @@
this.context = code.context();
}
- public boolean isSubsumedBy(Value conditionValue, BasicBlock block, BasicBlock other) {
- return isSubsumedBy(conditionValue, block.iterator(), other.iterator(), null);
+ public boolean isSubsumedBy(
+ Value conditionValue, Position conditionPosition, BasicBlock block, BasicBlock other) {
+ return isSubsumedBy(
+ conditionValue, conditionPosition, block.iterator(), other.iterator(), null);
}
private boolean dependsOnConditionValue(Value conditionValue, Instruction instruction) {
@@ -81,13 +84,21 @@
}
private Instruction skipNonDependentInstructionsUntil(
- InstructionIterator iterator, Value conditionValue, Predicate<Instruction> predicate) {
+ InstructionIterator iterator,
+ Value conditionValue,
+ Position position,
+ Predicate<Instruction> predicate) {
return iterator.nextUntil(
- predicate.or(i -> i.isJumpInstruction() || dependsOnConditionValue(conditionValue, i)));
+ predicate.or(
+ i ->
+ i.isJumpInstruction()
+ || (i.isDebugPosition() && !i.getPosition().equals(position))
+ || dependsOnConditionValue(conditionValue, i)));
}
private boolean isSubsumedBy(
Value conditionValue,
+ Position conditionPosition,
InstructionIterator iterator,
InstructionIterator otherIterator,
Set<BasicBlock> visited) {
@@ -95,7 +106,7 @@
// outside the block itself) that do not have side effects.
Instruction instruction =
skipNonDependentInstructionsUntil(
- iterator, conditionValue, this::isNonLocalDefinitionOrSideEffecting);
+ iterator, conditionValue, conditionPosition, this::isNonLocalDefinitionOrSideEffecting);
// If the instruction defines a value with non-local usages, then we would need a dominator
// analysis to verify that all these non-local usages can in fact be replaced by a value
@@ -107,7 +118,7 @@
// Skip over non-throwing instructions in the other block.
Instruction otherInstruction =
skipNonDependentInstructionsUntil(
- otherIterator, conditionValue, this::instructionMayHaveSideEffects);
+ otherIterator, conditionValue, conditionPosition, this::instructionMayHaveSideEffects);
assert otherInstruction != null;
if (instruction.isGoto()) {
@@ -139,7 +150,8 @@
visited = SetUtils.newIdentityHashSet(instruction.getBlock());
}
if (visited.add(targetBlock)) {
- return isSubsumedBy(conditionValue, targetBlock.iterator(), otherIterator, visited);
+ return isSubsumedBy(
+ conditionValue, conditionPosition, targetBlock.iterator(), otherIterator, visited);
}
// Guard against cycles in the control flow graph.
return false;
@@ -164,7 +176,10 @@
otherIterator = targetBlock.iterator();
otherInstruction =
skipNonDependentInstructionsUntil(
- otherIterator, conditionValue, this::instructionMayHaveSideEffects);
+ otherIterator,
+ conditionValue,
+ conditionPosition,
+ this::instructionMayHaveSideEffects);
// If following the first goto instruction leads to another goto instruction, then we need to
// keep track of the set of visited blocks to guard against cycles in the control flow graph.
@@ -188,7 +203,7 @@
|| otherSingleTarget.getDefinition().getOptimizationInfo().mayHaveSideEffects()) {
return false;
}
- return isSubsumedBy(conditionValue, iterator, otherIterator, visited);
+ return isSubsumedBy(conditionValue, conditionPosition, iterator, otherIterator, visited);
}
return false;
}
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/passes/BranchSimplifier.java b/src/main/java/com/android/tools/r8/ir/conversion/passes/BranchSimplifier.java
index 4696ec5..228eb0f 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/passes/BranchSimplifier.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/passes/BranchSimplifier.java
@@ -137,7 +137,8 @@
// Unable to determine which branch will be taken. Check if the true target can safely be
// rewritten to the false target.
if (behavioralSubsumption.isSubsumedBy(
- theIf.inValues().get(0), theIf.getTrueTarget(), theIf.fallthroughBlock())) {
+ theIf.inValues().get(0), theIf.getPosition(),
+ theIf.getTrueTarget(), theIf.fallthroughBlock())) {
simplifyIfWithKnownCondition(code, block, theIf, theIf.fallthroughBlock());
simplified = true;
}
@@ -837,7 +838,7 @@
if (switchCaseAnalyzer.switchCaseIsUnreachable(theSwitch, switchAbstractValue, i)) {
eliminator.markSwitchCaseForRemoval(i);
} else if (behavioralSubsumption.isSubsumedBy(
- theSwitch.value(), targetBlock, defaultTarget)) {
+ theSwitch.value(), theSwitch.getPosition(), targetBlock, defaultTarget)) {
eliminator.markSwitchCaseForRemoval(i);
hasSwitchCaseToDefaultRewrite = true;
}
diff --git a/src/test/java/com/android/tools/r8/debug/RegressB339064674Test.java b/src/test/java/com/android/tools/r8/debug/RegressB339064674Test.java
new file mode 100644
index 0000000..e2a2b26
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/debug/RegressB339064674Test.java
@@ -0,0 +1,20 @@
+// Copyright (c) 2024, 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.debug;
+
+import java.util.Arrays;
+
+public class RegressB339064674Test {
+
+ public static void main(String[] args) {
+ for (int i : Arrays.asList(0, 1)) {
+ int a1 = 1;
+ if (i == 0) {
+ continue;
+ }
+ int a2 = 2;
+ }
+ int a3 = 3;
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/debug/RegressB339064674TestRunner.java b/src/test/java/com/android/tools/r8/debug/RegressB339064674TestRunner.java
new file mode 100644
index 0000000..c8507ef
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/debug/RegressB339064674TestRunner.java
@@ -0,0 +1,106 @@
+// Copyright (c) 2024, 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.debug;
+
+import static org.junit.Assume.assumeTrue;
+
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.utils.AndroidApiLevel;
+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 RegressB339064674TestRunner extends DebugTestBase {
+
+ static final Class CLASS = RegressB339064674Test.class;
+ static final String NAME = CLASS.getCanonicalName();
+ static final String FILE = CLASS.getSimpleName() + ".java";
+ static final AndroidApiLevel minApi = AndroidApiLevel.B;
+ static final String EXPECTED = "";
+
+ private final TestParameters parameters;
+
+ @Parameters(name = "{0}")
+ public static TestParametersCollection setup() {
+ return TestParameters.builder().withAllRuntimes().build();
+ }
+
+ public RegressB339064674TestRunner(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
+ @Test
+ public void testReference() throws Throwable {
+ assumeTrue(parameters.isCfRuntime());
+ testForJvm(parameters)
+ .addProgramClasses(CLASS)
+ .run(parameters.getRuntime(), CLASS)
+ .assertSuccessWithOutput(EXPECTED)
+ .debugger(this::runDebugger);
+ }
+
+ @Test
+ public void testD8() throws Throwable {
+ testForD8(parameters.getBackend())
+ .setMinApi(minApi)
+ .addProgramClasses(CLASS)
+ .run(parameters.getRuntime(), CLASS)
+ .assertSuccessWithOutput(EXPECTED)
+ .debugger(this::runDebugger);
+ }
+
+ @Test
+ public void testR8() throws Throwable {
+ testForR8(parameters.getBackend())
+ .debug()
+ .addKeepMainRule(CLASS)
+ .addProgramClasses(CLASS)
+ .setMinApi(minApi)
+ .run(parameters.getRuntime(), CLASS)
+ .assertSuccessWithOutput(EXPECTED)
+ .debugger(this::runDebugger);
+ }
+
+ public void runDebugger(DebugTestConfig config) throws Throwable {
+ // The test ensures that when breaking inside a function and changing a local in the parent
+ // frame, that the new value is passed to the second invocation of the function.
+ // This ensures that no peephole optimizations will optimize if there is any debug information.
+ runDebugTest(
+ config,
+ NAME,
+ breakpoint(NAME, "main"),
+ run(),
+ checkLine(FILE, 11),
+ stepOver(),
+ checkLine(FILE, 12),
+ stepOver(),
+ checkLine(FILE, 13),
+ stepOver(),
+ checkLine(FILE, 14),
+ stepOver(),
+ checkLine(FILE, 11),
+ stepOver(),
+ checkLine(FILE, 12),
+ stepOver(),
+ checkLine(FILE, 13),
+ stepOver(),
+ checkLine(FILE, 16),
+ checkLocals("args", "i", "a1"),
+ checkNoLocal("a2"),
+ stepOver(),
+ checkLine(FILE, 17),
+ checkLocals("args"),
+ checkNoLocal("i"),
+ stepOver(),
+ checkLine(FILE, 11),
+ stepOver(),
+ checkLine(FILE, 18),
+ stepOver(),
+ checkLine(FILE, 19),
+ run());
+ }
+}
