Fix HInstruction::ReplaceInput(), allow no-op.
Allow HInstruction::ReplaceInput() to be called with
a `replacement` being the same as the old input and
do nothing in that case.
This is a follow-up to
https://android-review.googlesource.com/216923
where I erroneously assumed that it never happens.
Also adhere to the standard C++ std::forward_list<>
semantics in the single-element overload of
`IntrusiveForwardList<>::splice_after()`.
Bug: 28173563
Change-Id: I5cea14c212b1083f90ffe6b5b53324ad663d57d8
diff --git a/compiler/optimizing/nodes.cc b/compiler/optimizing/nodes.cc
index 7c6e931..fe75451 100644
--- a/compiler/optimizing/nodes.cc
+++ b/compiler/optimizing/nodes.cc
@@ -1022,8 +1022,11 @@
void HInstruction::ReplaceInput(HInstruction* replacement, size_t index) {
HUserRecord<HInstruction*> input_use = InputRecordAt(index);
+ if (input_use.GetInstruction() == replacement) {
+ // Nothing to do.
+ return;
+ }
HUseList<HInstruction*>::iterator before_use_node = input_use.GetBeforeUseNode();
- DCHECK(input_use.GetInstruction() != replacement);
// Note: fixup_end remains valid across splice_after().
auto fixup_end =
replacement->uses_.empty() ? replacement->uses_.begin() : ++replacement->uses_.begin();
diff --git a/compiler/utils/intrusive_forward_list.h b/compiler/utils/intrusive_forward_list.h
index a90b0f5..ec2c087 100644
--- a/compiler/utils/intrusive_forward_list.h
+++ b/compiler/utils/intrusive_forward_list.h
@@ -221,6 +221,13 @@
}
// Splice the element after `i`.
void splice_after(const_iterator position, IntrusiveForwardList& src, const_iterator i) {
+ // The standard specifies that this version does nothing if `position == i`
+ // or `position == ++i`. We must handle the latter here because the overload
+ // `splice_after(position, src, first, last)` does not allow `position` inside
+ // the range `(first, last)`.
+ if (++const_iterator(i) == position) {
+ return;
+ }
const_iterator last = i;
std::advance(last, 2);
splice_after(position, src, i, last);
diff --git a/compiler/utils/intrusive_forward_list_test.cc b/compiler/utils/intrusive_forward_list_test.cc
index d40ff3a..517142e 100644
--- a/compiler/utils/intrusive_forward_list_test.cc
+++ b/compiler/utils/intrusive_forward_list_test.cc
@@ -369,8 +369,33 @@
check.assign({8});
ASSERT_LISTS_EQUAL(check, ifl1);
+
+ // Move the first element of ref1/ifl1 to the beginning of ref1/ifl1 (do nothing).
+ ref1.splice_after(ref1.before_begin(), ref1, ref1.before_begin());
+ ifl1.splice_after(ifl1.before_begin(), ifl1, ifl1.before_begin());
+ ASSERT_LISTS_EQUAL(ref1, ifl1);
+ ASSERT_LISTS_EQUAL(check, ifl1);
+
+ // Move the first element of ref2/ifl2 after itself (do nothing).
+ ref1.splice_after(ref1.begin(), ref1, ref1.before_begin());
+ ifl1.splice_after(ifl1.begin(), ifl1, ifl1.before_begin());
+ ASSERT_LISTS_EQUAL(ref1, ifl1);
+ ASSERT_LISTS_EQUAL(check, ifl1);
+
check.assign({ 1, 7, 7, 2, 3, 4, 5, 4 });
ASSERT_LISTS_EQUAL(check, ifl2);
+
+ // Move the first element of ref2/ifl2 to the beginning of ref2/ifl2 (do nothing).
+ ref2.splice_after(ref2.before_begin(), ref2, ref2.before_begin());
+ ifl2.splice_after(ifl2.before_begin(), ifl2, ifl2.before_begin());
+ ASSERT_LISTS_EQUAL(ref2, ifl2);
+ ASSERT_LISTS_EQUAL(check, ifl2);
+
+ // Move the first element of ref2/ifl2 after itself (do nothing).
+ ref2.splice_after(ref2.begin(), ref2, ref2.before_begin());
+ ifl2.splice_after(ifl2.begin(), ifl2, ifl2.before_begin());
+ ASSERT_LISTS_EQUAL(ref2, ifl2);
+ ASSERT_LISTS_EQUAL(check, ifl2);
}
TEST(IntrusiveForwardList, Remove) {
diff --git a/test/567-checker-compare/src/Main.java b/test/567-checker-compare/src/Main.java
index f95ff1a..8587950 100644
--- a/test/567-checker-compare/src/Main.java
+++ b/test/567-checker-compare/src/Main.java
@@ -16,6 +16,32 @@
public class Main {
+ public static boolean doThrow = false;
+
+ /// CHECK-START: void Main.$opt$noinline$testReplaceInputWithItself(int) intrinsics_recognition (after)
+ /// CHECK-DAG: <<ArgX:i\d+>> ParameterValue
+ /// CHECK-DAG: <<Method:[ij]\d+>> CurrentMethod
+ /// CHECK-DAG: <<Zero:i\d+>> IntConstant 0
+ /// CHECK-DAG: <<Cmp:i\d+>> InvokeStaticOrDirect [<<ArgX>>,<<Zero>>,<<Method>>] intrinsic:IntegerCompare
+ /// CHECK-DAG: GreaterThanOrEqual [<<Cmp>>,<<Zero>>]
+
+ /// CHECK-START: void Main.$opt$noinline$testReplaceInputWithItself(int) instruction_simplifier (after)
+ /// CHECK-DAG: <<ArgX:i\d+>> ParameterValue
+ /// CHECK-DAG: <<Zero:i\d+>> IntConstant 0
+ /// CHECK-DAG: GreaterThanOrEqual [<<ArgX>>,<<Zero>>]
+
+ public static void $opt$noinline$testReplaceInputWithItself(int x) {
+ if (doThrow) { throw new Error(); }
+
+ // The instruction simplifier first replaces Integer.compare(x, 0) with Compare HIR
+ // and then merges the Compare into the GreaterThanOrEqual. This is a regression
+ // test that to check that it is allowed to replace the second input of the
+ // GreaterThanOrEqual, i.e. <<Zero>>, with the very same instruction.
+ if (Integer.compare(x, 0) < 0) {
+ System.out.println("OOOPS");
+ }
+ }
+
/// CHECK-START: int Main.compareBooleans(boolean, boolean) intrinsics_recognition (after)
/// CHECK-DAG: <<Method:[ij]\d+>> CurrentMethod
/// CHECK-DAG: <<Zero:i\d+>> IntConstant 0
@@ -890,6 +916,8 @@
public static void main(String args[]) {
+ $opt$noinline$testReplaceInputWithItself(42);
+
testCompareBooleans();
testCompareBytes();
testCompareShorts();
