diff options
-rw-r--r-- | compiler/optimizing/instruction_simplifier.cc | 20 | ||||
-rw-r--r-- | test/699-checker-string-append2/expected.txt | 1 | ||||
-rw-r--r-- | test/699-checker-string-append2/info.txt | 1 | ||||
-rw-r--r-- | test/699-checker-string-append2/smali/B146014745.smali | 163 | ||||
-rw-r--r-- | test/699-checker-string-append2/src/Main.java | 40 | ||||
-rw-r--r-- | test/knownfailures.json | 3 |
6 files changed, 218 insertions, 10 deletions
diff --git a/compiler/optimizing/instruction_simplifier.cc b/compiler/optimizing/instruction_simplifier.cc index d272bfaec9..17b5ad5376 100644 --- a/compiler/optimizing/instruction_simplifier.cc +++ b/compiler/optimizing/instruction_simplifier.cc @@ -2489,6 +2489,10 @@ static bool TryReplaceStringBuilderAppend(HInvoke* invoke) { 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 @@ static bool TryReplaceStringBuilderAppend(HInvoke* invoke) { 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 0000000000..b0aad4deb5 --- /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 0000000000..1bd4a1a297 --- /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 0000000000..0a20b41a68 --- /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 0000000000..3753af603b --- /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 559bee759d..5584382611 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", |