7179138: Incorrect result with String concatenation optimization

Summary: check for and skip diamond shaped NULL check code for the result of toString()
Reviewed-by: twisti, roland

src/share/vm/opto/stringopts.cpp

@@ -112,6 +112,7 @@ class StringConcat : public ResourceObj {
     _arguments->ins_req(0, value);
     _mode.insert_before(0, mode);
   }
+
   void push_string(Node* value) {
     push(value, StringMode);
   }
@@ -125,9 +126,56 @@ class StringConcat : public ResourceObj {
     push(value, CharMode);
   }
 
+  static bool is_SB_toString(Node* call) {
+    if (call->is_CallStaticJava()) {
+      CallStaticJavaNode* csj = call->as_CallStaticJava();
+      ciMethod* m = csj->method();
+      if (m != NULL &&
+          (m->intrinsic_id() == vmIntrinsics::_StringBuilder_toString ||
+           m->intrinsic_id() == vmIntrinsics::_StringBuffer_toString)) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  static Node* skip_string_null_check(Node* value) {
+    // Look for a diamond shaped Null check of toString() result
+    // (could be code from String.valueOf()):
+    // (Proj == NULL) ? "null":"CastPP(Proj)#NotNULL
+    if (value->is_Phi()) {
+      int true_path = value->as_Phi()->is_diamond_phi();
+      if (true_path != 0) {
+        // phi->region->if_proj->ifnode->bool
+        BoolNode* b = value->in(0)->in(1)->in(0)->in(1)->as_Bool();
+        Node* cmp = b->in(1);
+        Node* v1 = cmp->in(1);
+        Node* v2 = cmp->in(2);
+        // Null check of the return of toString which can simply be skipped.
+        if (b->_test._test == BoolTest::ne &&
+            v2->bottom_type() == TypePtr::NULL_PTR &&
+            value->in(true_path)->Opcode() == Op_CastPP &&
+            value->in(true_path)->in(1) == v1 &&
+            v1->is_Proj() && is_SB_toString(v1->in(0))) {
+          return v1;
+        }
+      }
+    }
+    return value;
+  }
+
   Node* argument(int i) {
     return _arguments->in(i);
   }
+  Node* argument_uncast(int i) {
+    Node* arg = argument(i);
+    int amode = mode(i);
+    if (amode == StringConcat::StringMode ||
+        amode == StringConcat::StringNullCheckMode) {
+      arg = skip_string_null_check(arg);
+    }
+    return arg;
+  }
   void set_argument(int i, Node* value) {
     _arguments->set_req(i, value);
   }
@@ -206,9 +254,11 @@ class StringConcat : public ResourceObj {
 
 
 void StringConcat::eliminate_unneeded_control() {
-  eliminate_initialize(begin()->initialization());
   for (uint i = 0; i < _control.size(); i++) {
     Node* n = _control.at(i);
+    if (n->is_Allocate()) {
+      eliminate_initialize(n->as_Allocate()->initialization());
+    }
     if (n->is_Call()) {
       if (n != _end) {
         eliminate_call(n->as_Call());
@@ -239,14 +289,15 @@ StringConcat* StringConcat::merge(StringConcat* other, Node* arg) {
   assert(result->_control.contains(other->_end), "what?");
   assert(result->_control.contains(_begin), "what?");
   for (int x = 0; x < num_arguments(); x++) {
-    if (argument(x) == arg) {
+    Node* argx = argument_uncast(x);
+    if (argx == arg) {
       // replace the toString result with the all the arguments that
       // made up the other StringConcat
       for (int y = 0; y < other->num_arguments(); y++) {
         result->append(other->argument(y), other->mode(y));
       }
     } else {
-      result->append(argument(x), mode(x));
+      result->append(argx, mode(x));
     }
   }
   result->set_allocation(other->_begin);
@@ -327,14 +378,9 @@ Node_List PhaseStringOpts::collect_toString_calls() {
 
   while (worklist.size() > 0) {
     Node* ctrl = worklist.pop();
-    if (ctrl->is_CallStaticJava()) {
+    if (StringConcat::is_SB_toString(ctrl)) {
       CallStaticJavaNode* csj = ctrl->as_CallStaticJava();
-      ciMethod* m = csj->method();
-      if (m != NULL &&
-          (m->intrinsic_id() == vmIntrinsics::_StringBuffer_toString ||
-           m->intrinsic_id() == vmIntrinsics::_StringBuilder_toString)) {
-        string_calls.push(csj);
-      }
+      string_calls.push(csj);
     }
     if (ctrl->in(0) != NULL && !_visited.test_set(ctrl->in(0)->_idx)) {
       worklist.push(ctrl->in(0));
@@ -550,44 +596,40 @@ PhaseStringOpts::PhaseStringOpts(PhaseGVN* gvn, Unique_Node_List*):
   for (int c = 0; c < concats.length(); c++) {
     StringConcat* sc = concats.at(c);
     for (int i = 0; i < sc->num_arguments(); i++) {
-      Node* arg = sc->argument(i);
-      if (arg->is_Proj() && arg->in(0)->is_CallStaticJava()) {
+      Node* arg = sc->argument_uncast(i);
+      if (arg->is_Proj() && StringConcat::is_SB_toString(arg->in(0))) {
         CallStaticJavaNode* csj = arg->in(0)->as_CallStaticJava();
-        if (csj->method() != NULL &&
-            (csj->method()->intrinsic_id() == vmIntrinsics::_StringBuilder_toString ||
-             csj->method()->intrinsic_id() == vmIntrinsics::_StringBuffer_toString)) {
-          for (int o = 0; o < concats.length(); o++) {
-            if (c == o) continue;
-            StringConcat* other = concats.at(o);
-            if (other->end() == csj) {
+        for (int o = 0; o < concats.length(); o++) {
+          if (c == o) continue;
+          StringConcat* other = concats.at(o);
+          if (other->end() == csj) {
 #ifndef PRODUCT
-              if (PrintOptimizeStringConcat) {
-                tty->print_cr("considering stacked concats");
-              }
+            if (PrintOptimizeStringConcat) {
+              tty->print_cr("considering stacked concats");
+            }
 #endif
 
-              StringConcat* merged = sc->merge(other, arg);
-              if (merged->validate_control_flow()) {
+            StringConcat* merged = sc->merge(other, arg);
+            if (merged->validate_control_flow()) {
 #ifndef PRODUCT
-                if (PrintOptimizeStringConcat) {
-                  tty->print_cr("stacking would succeed");
-                }
+              if (PrintOptimizeStringConcat) {
+                tty->print_cr("stacking would succeed");
+              }
 #endif
-                if (c < o) {
-                  concats.remove_at(o);
-                  concats.at_put(c, merged);
-                } else {
-                  concats.remove_at(c);
-                  concats.at_put(o, merged);
-                }
-                goto restart;
+              if (c < o) {
+                concats.remove_at(o);
+                concats.at_put(c, merged);
               } else {
+                concats.remove_at(c);
+                concats.at_put(o, merged);
+              }
+              goto restart;
+            } else {
 #ifndef PRODUCT
-                if (PrintOptimizeStringConcat) {
-                  tty->print_cr("stacking would fail");
-                }
-#endif
+              if (PrintOptimizeStringConcat) {
+                tty->print_cr("stacking would fail");
               }
+#endif
             }
           }
         }

test/compiler/7179138/Test7179138_1.java

+/*
+ * Copyright 2012 Skip Balk.  All Rights Reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 7179138
+ * @summary Incorrect result with String concatenation optimization
+ * @run main/othervm -Xbatch -XX:+IgnoreUnrecognizedVMOptions -XX:-TieredCompilation Test7179138_1
+ *
+ * @author Skip Balk
+ */
+
+public class Test7179138_1 {
+    public static void main(String[] args) throws Exception {
+        System.out.println("Java Version: " + System.getProperty("java.vm.version"));
+        long[] durations = new long[60];
+        for (int i = 0; i < 100000; i++) {
+            // this empty for-loop is required to reproduce this bug
+            for (long duration : durations) {
+                // do nothing
+            }
+            {
+                String s = "test";
+                int len = s.length();
+
+                s = new StringBuilder(String.valueOf(s)).append(s).toString();
+                len = len + len;
+
+                s = new StringBuilder(String.valueOf(s)).append(s).toString();
+                len = len + len;
+
+                s = new StringBuilder(String.valueOf(s)).append(s).toString();
+                len = len + len;
+
+                if (s.length() != len) {
+                    System.out.println("Failed at iteration: " + i);
+                    System.out.println("Length mismatch: " + s.length() + " <> " + len);
+                    System.out.println("Expected: \"" + "test" + "test" + "test" + "test" + "test" + "test" + "test" + "test" + "\"");
+                    System.out.println("Actual:   \"" + s + "\"");
+                    System.exit(97);
+                }
+            }
+        }
+    }
+}
+

test/compiler/7179138/Test7179138_2.java

+/*
+ * Copyright 2012 Skip Balk.  All Rights Reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 7179138
+ * @summary Incorrect result with String concatenation optimization
+ * @run main/othervm -Xbatch -XX:+IgnoreUnrecognizedVMOptions -XX:-TieredCompilation Test7179138_2
+ *
+ * @author Skip Balk
+ */
+
+public class Test7179138_2 {
+    public static void main(String[] args) throws Exception {
+        System.out.println("Java Version: " + System.getProperty("java.vm.version"));
+        long[] durations = new long[60];
+        for (int i = 0; i < 100000; i++) {
+            // this empty for-loop is required to reproduce this bug
+            for (long duration : durations) {
+                // do nothing
+            }
+            {
+                String s = "test";
+                int len = s.length();
+
+                s = s + s;
+                len = len + len;
+
+                s = s + s;
+                len = len + len;
+
+                s = s + s;
+                len = len + len;
+
+                if (s.length() != len) {
+                    System.out.println("Failed at iteration: " + i);
+                    System.out.println("Length mismatch: " + s.length() + " <> " + len);
+                    System.out.println("Expected: \"" + "test" + "test" + "test" + "test" + "test" + "test" + "test" + "test" + "\"");
+                    System.out.println("Actual:   \"" + s + "\"");
+                    System.exit(0);
+                }
+            }
+        }
+    }
+}
+