8081823: C2 performs unsigned comparison against -1
authorroland
Mon, 08 Jun 2015 18:35:17 +0200
changeset 31132 328ac96a30d6
parent 31131 d92082b3ef93
child 31133 3e7542b42a61
8081823: C2 performs unsigned comparison against -1 Summary: x <= 0 || x > 0 wrongly folded as (x-1) >u -1 Reviewed-by: kvn, vlivanov
hotspot/src/share/vm/opto/ifnode.cpp
hotspot/test/compiler/rangechecks/TestBadFoldCompare.java
--- a/hotspot/src/share/vm/opto/ifnode.cpp	Thu Jun 04 16:19:22 2015 +0200
+++ b/hotspot/src/share/vm/opto/ifnode.cpp	Mon Jun 08 18:35:17 2015 +0200
@@ -817,19 +817,78 @@
   BoolTest::mask hi_test = this_bool->_test._test;
   BoolTest::mask cond = hi_test;
 
+  // convert:
+  //
+  //          dom_bool = x {<,<=,>,>=} a
+  //                           / \
+  //     proj = {True,False}  /   \ otherproj = {False,True}
+  //                         /
+  //        this_bool = x {<,<=} b
+  //                       / \
+  //  fail = {True,False} /   \ success = {False,True}
+  //                     /
+  //
+  // (Second test guaranteed canonicalized, first one may not have
+  // been canonicalized yet)
+  //
+  // into:
+  //
+  // cond = (x - lo) {<u,<=u,>u,>=u} adjusted_lim
+  //                       / \
+  //                 fail /   \ success
+  //                     /
+  //
+
   // Figure out which of the two tests sets the upper bound and which
   // sets the lower bound if any.
+  Node* adjusted_lim = NULL;
   if (hi_type->_lo > lo_type->_hi && hi_type->_hi == max_jint && lo_type->_lo == min_jint) {
-
     assert((dom_bool->_test.is_less() && !proj->_con) ||
            (dom_bool->_test.is_greater() && proj->_con), "incorrect test");
     // this test was canonicalized
     assert(this_bool->_test.is_less() && fail->_con, "incorrect test");
 
+    // this_bool = <
+    //   dom_bool = >= (proj = True) or dom_bool = < (proj = False)
+    //     x in [a, b[ on the fail (= True) projection, b > a-1 (because of hi_type->_lo > lo_type->_hi test above):
+    //     lo = a, hi = b, adjusted_lim = b-a, cond = <u
+    //   dom_bool = > (proj = True) or dom_bool = <= (proj = False)
+    //     x in ]a, b[ on the fail (= True) projection, b > a:
+    //     lo = a+1, hi = b, adjusted_lim = b-a-1, cond = <u
+    // this_bool = <=
+    //   dom_bool = >= (proj = True) or dom_bool = < (proj = False)
+    //     x in [a, b] on the fail (= True) projection, b+1 > a-1:
+    //     lo = a, hi = b, adjusted_lim = b-a, cond = <=u
+    //   dom_bool = > (proj = True) or dom_bool = <= (proj = False)
+    //     x in ]a, b] on the fail (= True) projection b+1 > a:
+    //     lo = a+1, hi = b, adjusted_lim = b-a, cond = <u
+    //     lo = a+1, hi = b, adjusted_lim = b-a-1, cond = <=u doesn't work because a = b is possible, then hi-lo = -1
+
     if (lo_test == BoolTest::gt || lo_test == BoolTest::le) {
+      if (hi_test == BoolTest::le) {
+        adjusted_lim = igvn->transform(new SubINode(hi, lo));
+        cond = BoolTest::lt;
+      }
       lo = igvn->transform(new AddINode(lo, igvn->intcon(1)));
     }
   } else if (lo_type->_lo > hi_type->_hi && lo_type->_hi == max_jint && hi_type->_lo == min_jint) {
+
+    // this_bool = <
+    //   dom_bool = < (proj = True) or dom_bool = >= (proj = False)
+    //     x in [b, a[ on the fail (= False) projection, a > b-1 (because of lo_type->_lo > hi_type->_hi above):
+    //     lo = b, hi = a, adjusted_lim = a-b, cond = >=u
+    //   dom_bool = <= (proj = True) or dom_bool = > (proj = False)
+    //     x in [b, a] on the fail (= False) projection, a+1 > b-1:
+    //     lo = b, hi = a, adjusted_lim = a-b, cond = >u
+    // this_bool = <=
+    //   dom_bool = < (proj = True) or dom_bool = >= (proj = False)
+    //     x in ]b, a[ on the fail (= False) projection, a > b:
+    //     lo = b+1, hi = a, adjusted_lim = a-b-1, cond = >=u
+    //   dom_bool = <= (proj = True) or dom_bool = > (proj = False)
+    //     x in ]b, a] on the fail (= False) projection, a+1 > b:
+    //     lo = b+1, hi = a, adjusted_lim = a-b, cond = >=u
+    //     lo = b+1, hi = a, adjusted_lim = a-b-1, cond = >u doesn't work because a = b is possible, then hi-lo = -1
+
     swap(lo, hi);
     swap(lo_type, hi_type);
     swap(lo_test, hi_test);
@@ -842,6 +901,10 @@
     cond = (hi_test == BoolTest::le || hi_test == BoolTest::gt) ? BoolTest::gt : BoolTest::ge;
 
     if (lo_test == BoolTest::le) {
+      if (cond == BoolTest::gt) {
+        adjusted_lim = igvn->transform(new SubINode(hi, lo));
+        cond = BoolTest::ge;
+      }
       lo = igvn->transform(new AddINode(lo, igvn->intcon(1)));
     }
 
@@ -860,7 +923,6 @@
         }
       }
     }
-
     lo = NULL;
     hi = NULL;
   }
@@ -868,12 +930,13 @@
   if (lo && hi) {
     // Merge the two compares into a single unsigned compare by building (CmpU (n - lo) (hi - lo))
     Node* adjusted_val = igvn->transform(new SubINode(n,  lo));
-    Node* adjusted_lim = igvn->transform(new SubINode(hi, lo));
+    if (adjusted_lim == NULL) {
+      adjusted_lim = igvn->transform(new SubINode(hi, lo));
+    }
     Node* newcmp = igvn->transform(new CmpUNode(adjusted_val, adjusted_lim));
     Node* newbool = igvn->transform(new BoolNode(newcmp, cond));
 
-    igvn->is_IterGVN()->replace_input_of(dom_iff, 1, igvn->intcon(proj->_con));
-    igvn->hash_delete(this);
+    igvn->replace_input_of(dom_iff, 1, igvn->intcon(proj->_con));
     set_req(1, newbool);
 
     return true;
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/hotspot/test/compiler/rangechecks/TestBadFoldCompare.java	Mon Jun 08 18:35:17 2015 +0200
@@ -0,0 +1,94 @@
+/*
+ * Copyright (c) 2015, Oracle and/or its affiliates. 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 8085832
+ * @summary x <= 0 || x > 0 wrongly folded as (x-1) >u -1
+ * @run main/othervm -XX:-BackgroundCompilation -XX:-UseOnStackReplacement TestBadFoldCompare
+ */
+
+public class TestBadFoldCompare {
+
+    static boolean test1_taken;
+
+    static void helper1(int i, int a, int b, boolean flag) {
+        if (flag) {
+            if (i <= a || i > b) {
+                test1_taken = true;
+            }
+        }
+    }
+
+    static void test1(int i, boolean flag) {
+        helper1(i, 0, 0, flag);
+    }
+
+    static boolean test2_taken;
+
+    static void helper2(int i, int a, int b, boolean flag) {
+        if (flag) {
+            if (i > b || i <= a) {
+                test2_taken = true;
+            }
+        }
+    }
+
+    static void test2(int i, boolean flag) {
+        helper2(i, 0, 0, flag);
+    }
+
+    static public void main(String[] args) {
+        boolean success = true;
+
+        for (int i = 0; i < 20000; i++) {
+            helper1(5, 0,  10, (i%2)==0);
+            helper1(-1, 0,  10, (i%2)==0);
+            helper1(15, 0,  10, (i%2)==0);
+            test1(0, false);
+        }
+        test1_taken = false;
+        test1(0, true);
+        if (!test1_taken) {
+            System.out.println("Test1 failed");
+            success = false;
+        }
+
+        for (int i = 0; i < 20000; i++) {
+            helper2(5, 0,  10, (i%2)==0);
+            helper2(-1, 0,  10, (i%2)==0);
+            helper2(15, 0,  10, (i%2)==0);
+            test2(0, false);
+        }
+        test2_taken = false;
+        test2(0, true);
+
+        if (!test2_taken) {
+            System.out.println("Test2 failed");
+            success = false;
+        }
+        if (!success) {
+            throw new RuntimeException("Some tests failed");
+        }
+    }
+}