diff src/share/vm/opto/stringopts.cpp @ 6186:751bd303aa45

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
author kvn
date Tue, 26 Jun 2012 09:06:16 -0700
parents 8f972594effc
children ed21db7b3fda
line wrap: on
line diff
--- a/src/share/vm/opto/stringopts.cpp	Fri Jun 22 10:40:48 2012 -0700
+++ b/src/share/vm/opto/stringopts.cpp	Tue Jun 26 09:06:16 2012 -0700
@@ -112,6 +112,7 @@
     _arguments->ins_req(0, value);
     _mode.insert_before(0, mode);
   }
+
   void push_string(Node* value) {
     push(value, StringMode);
   }
@@ -125,9 +126,56 @@
     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 @@
 
 
 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 @@
   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 @@
 
   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 @@
   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");
-                }
+              if (PrintOptimizeStringConcat) {
+                tty->print_cr("stacking would fail");
+              }
 #endif
-              }
             }
           }
         }