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

4 files changed, 64 insertions, 1 deletions

diff --git a/compiler/optimizing/nodes.cc b/compiler/optimizing/nodes.cc
index 7c6e9318fb..fe75451ad2 100644
--- a/compiler/optimizing/nodes.cc
+++ b/compiler/optimizing/nodes.cc
@@ -1022,8 +1022,11 @@ void HInstruction::ReplaceWith(HInstruction* other) {
 
 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 a90b0f538c..ec2c08722c 100644
--- a/compiler/utils/intrusive_forward_list.h
+++ b/compiler/utils/intrusive_forward_list.h
@@ -221,6 +221,13 @@ class IntrusiveForwardList {
   }
   // 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 d40ff3a721..517142e1b5 100644
--- a/compiler/utils/intrusive_forward_list_test.cc
+++ b/compiler/utils/intrusive_forward_list_test.cc
@@ -369,8 +369,33 @@ TEST(IntrusiveForwardList, SpliceAfter) {
 
   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 f95ff1a7db..85879500d9 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 class Main {
 
 
   public static void main(String args[]) {
+    $opt$noinline$testReplaceInputWithItself(42);
+
     testCompareBooleans();
     testCompareBytes();
     testCompareShorts();