Fix StringBuilder append assumptions.
Do not rely on use ordering, it can be different because of
SimplifyReturnThis(); just use HBackwardInstructionIterator.
Instead of checking that we find a StringBuilder.toString(),
check that it is the invoke we're trying to simplify.
Add regression test 699-checker-string-append2.
Test: testrunner.py --host --jvm -t 699-checker-string-append2
Bug: 19575890
Bug: 146014745
Change-Id: I7b16f376c16ba5a4107e9718e0acf17d82280f54
diff --git a/compiler/optimizing/instruction_simplifier.cc b/compiler/optimizing/instruction_simplifier.cc
index d272bfa..17b5ad5 100644
--- a/compiler/optimizing/instruction_simplifier.cc
+++ b/compiler/optimizing/instruction_simplifier.cc
@@ -2489,6 +2489,10 @@
if (use.GetUser()->GetBlock() != block) {
return false;
}
+ // The append pattern uses the StringBuilder only as the first argument.
+ if (use.GetIndex() != 0u) {
+ return false;
+ }
}
// Collect args and check for unexpected uses.
@@ -2500,17 +2504,15 @@
uint32_t format = 0u;
uint32_t num_args = 0u;
HInstruction* args[StringBuilderAppend::kMaxArgs]; // Added in reverse order.
- for (const HUseListNode<HInstruction*>& use : sb->GetUses()) {
- // The append pattern uses the StringBuilder only as the first argument.
- if (use.GetIndex() != 0u) {
- return false;
+ for (HBackwardInstructionIterator iter(block->GetInstructions()); !iter.Done(); iter.Advance()) {
+ HInstruction* user = iter.Current();
+ // Instructions of interest apply to `sb`, skip those that do not involve `sb`.
+ if (user->InputCount() == 0u || user->InputAt(0u) != sb) {
+ continue;
}
- // We see the uses in reverse order because they are inserted at the front
- // of the singly-linked list, so the StringBuilder.append() must come first.
- HInstruction* user = use.GetUser();
+ // We visit the uses in reverse order, so the StringBuilder.toString() must come first.
if (!seen_to_string) {
- if (user->IsInvokeVirtual() &&
- user->AsInvokeVirtual()->GetIntrinsic() == Intrinsics::kStringBuilderToString) {
+ if (user == invoke) {
seen_to_string = true;
continue;
} else {
diff --git a/test/699-checker-string-append2/expected.txt b/test/699-checker-string-append2/expected.txt
new file mode 100644
index 0000000..b0aad4d
--- /dev/null
+++ b/test/699-checker-string-append2/expected.txt
@@ -0,0 +1 @@
+passed
diff --git a/test/699-checker-string-append2/info.txt b/test/699-checker-string-append2/info.txt
new file mode 100644
index 0000000..1bd4a1a
--- /dev/null
+++ b/test/699-checker-string-append2/info.txt
@@ -0,0 +1 @@
+Regression tests for String append pattern recognition bugs. b/146014745
diff --git a/test/699-checker-string-append2/smali/B146014745.smali b/test/699-checker-string-append2/smali/B146014745.smali
new file mode 100644
index 0000000..0a20b41
--- /dev/null
+++ b/test/699-checker-string-append2/smali/B146014745.smali
@@ -0,0 +1,163 @@
+# Copyright (C) 2019 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+.class public LB146014745;
+.super Ljava/lang/Object;
+
+## CHECK-START: java.lang.String B146014745.$noinline$testAppend1(java.lang.String, int) instruction_simplifier (before)
+## CHECK-NOT: StringBuilderAppend
+
+## CHECK-START: java.lang.String B146014745.$noinline$testAppend1(java.lang.String, int) instruction_simplifier (after)
+## CHECK: StringBuilderAppend
+
+.method public static $noinline$testAppend1(Ljava/lang/String;I)Ljava/lang/String;
+ .registers 4
+# StringBuilder sb = new StringBuilder();
+ new-instance v0, Ljava/lang/StringBuilder;
+ invoke-direct {v0}, Ljava/lang/StringBuilder;-><init>()V
+# sb.append(s).append(i);
+ invoke-virtual {v0, p0}, Ljava/lang/StringBuilder;->append(Ljava/lang/String;)Ljava/lang/StringBuilder;
+ move-result-object v1
+ invoke-virtual {v1, p1}, Ljava/lang/StringBuilder;->append(I)Ljava/lang/StringBuilder;
+# return sb.append(s).append(i).toString();
+ invoke-virtual {v0, p0}, Ljava/lang/StringBuilder;->append(Ljava/lang/String;)Ljava/lang/StringBuilder;
+ move-result-object v1
+ invoke-virtual {v1, p1}, Ljava/lang/StringBuilder;->append(I)Ljava/lang/StringBuilder;
+ move-result-object v1
+ invoke-virtual {v1}, Ljava/lang/StringBuilder;->toString()Ljava/lang/String;
+ move-result-object v1
+ return-object v1
+.end method
+
+## CHECK-START: java.lang.String B146014745.$noinline$testAppend2(java.lang.String, int) instruction_simplifier (after)
+## CHECK-NOT: StringBuilderAppend
+
+.method public static $noinline$testAppend2(Ljava/lang/String;I)Ljava/lang/String;
+ .registers 4
+# StringBuilder sb = new StringBuilder();
+ new-instance v0, Ljava/lang/StringBuilder;
+ invoke-direct {v0}, Ljava/lang/StringBuilder;-><init>()V
+# String s2 = sb.append(s).append(i).toString();
+ invoke-virtual {v0, p0}, Ljava/lang/StringBuilder;->append(Ljava/lang/String;)Ljava/lang/StringBuilder;
+ move-result-object v1
+ invoke-virtual {v1, p1}, Ljava/lang/StringBuilder;->append(I)Ljava/lang/StringBuilder;
+ move-result-object v1
+ invoke-virtual {v1}, Ljava/lang/StringBuilder;->toString()Ljava/lang/String;
+ move-result-object v1
+# return sb.append(s2).toString();
+ invoke-virtual {v0, v1}, Ljava/lang/StringBuilder;->append(Ljava/lang/String;)Ljava/lang/StringBuilder;
+ move-result-object v1
+ invoke-virtual {v1}, Ljava/lang/StringBuilder;->toString()Ljava/lang/String;
+ move-result-object v1
+ return-object v1
+.end method
+
+## CHECK-START: java.lang.String B146014745.$noinline$testAppend3(java.lang.String, int) instruction_simplifier (after)
+## CHECK-NOT: StringBuilderAppend
+
+.method public static $noinline$testAppend3(Ljava/lang/String;I)Ljava/lang/String;
+ .registers 5
+# StringBuilder sb = new StringBuilder();
+ new-instance v0, Ljava/lang/StringBuilder;
+ invoke-direct {v0}, Ljava/lang/StringBuilder;-><init>()V
+# String s2 = sb.append(s).toString();
+ invoke-virtual {v0, p0}, Ljava/lang/StringBuilder;->append(Ljava/lang/String;)Ljava/lang/StringBuilder;
+ move-result-object v2
+ invoke-virtual {v2}, Ljava/lang/StringBuilder;->toString()Ljava/lang/String;
+ move-result-object v2
+# return sb.append(i).append(s2).append(i);
+ invoke-virtual {v0, p1}, Ljava/lang/StringBuilder;->append(I)Ljava/lang/StringBuilder;
+ move-result-object v1
+ invoke-virtual {v1, v2}, Ljava/lang/StringBuilder;->append(Ljava/lang/String;)Ljava/lang/StringBuilder;
+ move-result-object v1
+ invoke-virtual {v1, p1}, Ljava/lang/StringBuilder;->append(I)Ljava/lang/StringBuilder;
+# return sb.toString();
+ invoke-virtual {v0}, Ljava/lang/StringBuilder;->toString()Ljava/lang/String;
+ move-result-object v1
+ return-object v1
+.end method
+
+# The following is a jasmin version.
+# Unfortunately, this would be translated without the required move-result-object,
+# instead using the register initialized by new-instance for subsequent calls.
+
+#.class public B146014745
+#.super java/lang/Object
+
+#.method public static $noinline$testAppend1(Ljava/lang/String;I)Ljava/lang/String;
+# .limit stack 3
+# .limit locals 2
+#; StringBuilder sb = new StringBuilder();
+# new java/lang/StringBuilder
+# dup
+# invokespecial java.lang.StringBuilder.<init>()V
+#; sb.append(s).append(i);
+# dup
+# aload_0
+# invokevirtual java.lang.StringBuilder.append(Ljava/lang/String;)Ljava/lang/StringBuilder;
+# iload_1
+# invokevirtual java.lang.StringBuilder.append(I)Ljava/lang/StringBuilder;
+# pop
+#; return sb.append(s).append(i).toString();
+# aload_0
+# invokevirtual java.lang.StringBuilder.append(Ljava/lang/String;)Ljava/lang/StringBuilder;
+# iload_1
+# invokevirtual java.lang.StringBuilder.append(I)Ljava/lang/StringBuilder;
+# invokevirtual java.lang.StringBuilder.toString()Ljava/lang/String;
+# areturn
+#.end method
+
+#.method public static $noinline$testAppend2(Ljava/lang/String;I)Ljava/lang/String;
+# .limit stack 3
+# .limit locals 2
+#; StringBuilder sb = new StringBuilder();
+# new java/lang/StringBuilder
+# dup
+# invokespecial java.lang.StringBuilder.<init>()V
+#; String s2 = sb.append(s).append(i).toString();
+# dup
+# aload_0
+# invokevirtual java.lang.StringBuilder.append(Ljava/lang/String;)Ljava/lang/StringBuilder;
+# iload_1
+# invokevirtual java.lang.StringBuilder.append(I)Ljava/lang/StringBuilder;
+# invokevirtual java.lang.StringBuilder.toString()Ljava/lang/String;
+#; return sb.append(s2).toString();
+# invokevirtual java.lang.StringBuilder.append(Ljava/lang/String;)Ljava/lang/StringBuilder;
+# invokevirtual java.lang.StringBuilder.toString()Ljava/lang/String;
+# areturn
+#.end method
+
+#.method public static $noinline$testAppend3(Ljava/lang/String;I)Ljava/lang/String;
+# .limit stack 3
+# .limit locals 3
+#; StringBuilder sb = new StringBuilder();
+# new java/lang/StringBuilder
+# dup
+# invokespecial java.lang.StringBuilder.<init>()V
+#; String s2 = sb.append(s).toString();
+# dup
+# aload_0
+# invokevirtual java.lang.StringBuilder.append(Ljava/lang/String;)Ljava/lang/StringBuilder;
+# invokevirtual java.lang.StringBuilder.toString()Ljava/lang/String;
+# astore_2
+#; return sb.append(i).append(s2).append(i);
+# iload_1
+# invokevirtual java.lang.StringBuilder.append(I)Ljava/lang/StringBuilder;
+# aload_2
+# invokevirtual java.lang.StringBuilder.append(Ljava/lang/String;)Ljava/lang/StringBuilder;
+# iload_1
+# invokevirtual java.lang.StringBuilder.append(I)Ljava/lang/StringBuilder;
+# invokevirtual java.lang.StringBuilder.toString()Ljava/lang/String;
+# areturn
+#.end method
diff --git a/test/699-checker-string-append2/src/Main.java b/test/699-checker-string-append2/src/Main.java
new file mode 100644
index 0000000..3753af6
--- /dev/null
+++ b/test/699-checker-string-append2/src/Main.java
@@ -0,0 +1,40 @@
+/*
+ * Copyright 2019 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import java.lang.reflect.Method;
+
+public class Main {
+ public static void main(String[] args) throws Exception {
+ Class<?> c = Class.forName("B146014745");
+ Method m1 = c.getDeclaredMethod("$noinline$testAppend1", String.class, int.class);
+ String b146014745_result1 = (String) m1.invoke(null, "x", 42);
+ assertEquals("x42x42", b146014745_result1);
+ Method m2 = c.getDeclaredMethod("$noinline$testAppend2", String.class, int.class);
+ String b146014745_result2 = (String) m2.invoke(null, "x", 42);
+ assertEquals("x42x42", b146014745_result2);
+ Method m3 = c.getDeclaredMethod("$noinline$testAppend3", String.class, int.class);
+ String b146014745_result3 = (String) m3.invoke(null, "x", 42);
+ assertEquals("x42x42", b146014745_result3);
+
+ System.out.println("passed");
+ }
+
+ public static void assertEquals(String expected, String actual) {
+ if (!expected.equals(actual)) {
+ throw new AssertionError("Expected: " + expected + ", actual: " + actual);
+ }
+ }
+}
diff --git a/test/knownfailures.json b/test/knownfailures.json
index 559bee7..5584382 100644
--- a/test/knownfailures.json
+++ b/test/knownfailures.json
@@ -1033,6 +1033,7 @@
"685-shifts",
"686-get-this",
"687-deopt",
+ "699-checker-string-append2",
"706-checker-scheduler",
"707-checker-invalid-profile",
"714-invoke-custom-lambda-metafactory",
@@ -1105,7 +1106,7 @@
],
"variant": "jvm",
"bug": "b/73888836",
- "description": ["Failing on RI. Needs further investigating."]
+ "description": ["Failing on RI. Needs further investigating. Some of these use smali."]
},
{
"tests": ["530-checker-peel-unroll",