8325417: [lworld] Incorrect re-execution state at uncommon traps emitted by acmp

src/hotspot/share/opto/compile.cpp

@@ -2401,10 +2401,6 @@ void Compile::process_for_unstable_if_traps(PhaseIterGVN& igvn) {
     if (next_bci != -1 && !modified) {
       assert(!_dead_node_list.test(unc->_idx), "changing a dead node!");
       JVMState* jvms = unc->jvms();
-      // TODO 8325106 TestLWorld::test118 will fail with -DWarmup=10000 -DVerifyIR=false
-      // because Parse::do_acmp uses Parse::do_if with custom ctrl_taken which puts uncommon traps on some paths and sets the next_bci incorrectly
-      // It also seems that TestOptimizeUnstableIf is not working. It still passes, even if this optimization is turned off.
-      if (jvms->should_reexecute()) continue;
       ciMethod* method = jvms->method();
       ciBytecodeStream iter(method);
 

src/hotspot/share/opto/parse.hpp

@@ -567,14 +567,14 @@ class Parse : public GraphKit {
   bool    seems_stable_comparison() const;
 
   void    do_ifnull(BoolTest::mask btest, Node* c);
-  void    do_if(BoolTest::mask btest, Node* c, bool new_path = false, Node** ctrl_taken = nullptr);
+  void    do_if(BoolTest::mask btest, Node* c, bool can_trap = true, bool new_path = false, Node** ctrl_taken = nullptr);
   void    do_acmp(BoolTest::mask btest, Node* left, Node* right);
   void    acmp_always_null_input(Node* input, const TypeOopPtr* tinput, BoolTest::mask btest, Node* eq_region);
   void    acmp_known_non_inline_type_input(Node* input, const TypeOopPtr* tinput, ProfilePtrKind input_ptr, ciKlass* input_type, BoolTest::mask btest, Node* eq_region);
   Node*   acmp_null_check(Node* input, const TypeOopPtr* tinput, ProfilePtrKind input_ptr, Node*& null_ctl);
   void    acmp_unknown_non_inline_type_input(Node* input, const TypeOopPtr* tinput, ProfilePtrKind input_ptr, BoolTest::mask btest, Node* eq_region);
   int     repush_if_args();
-  void    adjust_map_after_if(BoolTest::mask btest, Node* c, float prob, Block* path);
+  void    adjust_map_after_if(BoolTest::mask btest, Node* c, float prob, Block* path, bool can_trap = true);
   void    sharpen_type_after_if(BoolTest::mask btest,
                                 Node* con, const Type* tcon,
                                 Node* val, const Type* tval);

src/hotspot/share/opto/parse2.cpp

@@ -1792,7 +1792,7 @@ void Parse::do_ifnull(BoolTest::mask btest, Node *c) {
 }
 
 //------------------------------------do_if------------------------------------
-void Parse::do_if(BoolTest::mask btest, Node* c, bool new_path, Node** ctrl_taken) {
+void Parse::do_if(BoolTest::mask btest, Node* c, bool can_trap, bool new_path, Node** ctrl_taken) {
   int target_bci = iter().get_dest();
 
   Block* branch_block = successor_for_bci(target_bci);
@@ -1881,7 +1881,7 @@ void Parse::do_if(BoolTest::mask btest, Node* c, bool new_path, Node** ctrl_take
         branch_block->next_path_num();
       }
     } else {
-      adjust_map_after_if(taken_btest, c, prob, branch_block);
+      adjust_map_after_if(taken_btest, c, prob, branch_block, can_trap);
       if (!stopped()) {
         if (new_path) {
           // Merge by using a new path
@@ -1906,7 +1906,7 @@ void Parse::do_if(BoolTest::mask btest, Node* c, bool new_path, Node** ctrl_take
       next_block->next_path_num();
     }
   } else {
-    adjust_map_after_if(untaken_btest, c, untaken_prob, next_block);
+    adjust_map_after_if(untaken_btest, c, untaken_prob, next_block, can_trap);
   }
 }
 
@@ -2109,10 +2109,20 @@ void Parse::do_acmp(BoolTest::mask btest, Node* left, Node* right) {
     do_if(btest, cmp);
     return;
   }
+
+  // Don't add traps to unstable if branches because additional checks are required to
+  // decide if the operands are equal/substitutable and we therefore shouldn't prune
+  // branches for one if based on the profiling of the acmp branches.
+  // Also, OptimizeUnstableIf would set an incorrect re-rexecution state because it
+  // assumes that there is a 1-1 mapping between the if and the acmp branches and that
+  // hitting a trap means that we will take the corresponding acmp branch on re-execution.
+  const bool can_trap = true;
+
   Node* eq_region = nullptr;
   if (btest == BoolTest::eq) {
-    do_if(btest, cmp, true);
+    do_if(btest, cmp, !can_trap, true);
     if (stopped()) {
+      // Pointers are equal, operands must be equal
       return;
     }
   } else {
@@ -2121,8 +2131,8 @@ void Parse::do_acmp(BoolTest::mask btest, Node* left, Node* right) {
     eq_region = new RegionNode(3);
     {
       PreserveJVMState pjvms(this);
-      jvms()->set_should_reexecute(true);
-      do_if(btest, cmp, false, &is_not_equal);
+      // Pointers are not equal, but more checks are needed to determine if the operands are (not) substitutable
+      do_if(btest, cmp, !can_trap, false, &is_not_equal);
       if (!stopped()) {
         eq_region->init_req(1, control());
       }
@@ -2262,18 +2272,19 @@ void Parse::do_acmp(BoolTest::mask btest, Node* left, Node* right) {
   dec_sp(2);
 
   // Test the return value of ValueObjectMethods::isSubstitutable()
+  // This is the last check, do_if can emit traps now.
   Node* subst_cmp = _gvn.transform(new CmpINode(ret, intcon(1)));
   Node* ctl = C->top();
   if (btest == BoolTest::eq) {
     PreserveJVMState pjvms(this);
-    do_if(btest, subst_cmp);
+    do_if(btest, subst_cmp, can_trap);
     if (!stopped()) {
       ctl = control();
     }
   } else {
     assert(btest == BoolTest::ne, "only eq or ne");
     PreserveJVMState pjvms(this);
-    do_if(btest, subst_cmp, false, &ctl);
+    do_if(btest, subst_cmp, can_trap, false, &ctl);
     if (!stopped()) {
       eq_region->init_req(2, control());
       eq_io_phi->init_req(2, i_o());
@@ -2329,7 +2340,7 @@ void Parse::maybe_add_predicate_after_if(Block* path) {
 // branch, seeing how it constrains a tested value, and then
 // deciding if it's worth our while to encode this constraint
 // as graph nodes in the current abstract interpretation map.
-void Parse::adjust_map_after_if(BoolTest::mask btest, Node* c, float prob, Block* path) {
+void Parse::adjust_map_after_if(BoolTest::mask btest, Node* c, float prob, Block* path, bool can_trap) {
   if (!c->is_Cmp()) {
     maybe_add_predicate_after_if(path);
     return;
@@ -2341,7 +2352,7 @@ void Parse::adjust_map_after_if(BoolTest::mask btest, Node* c, float prob, Block
 
   bool is_fallthrough = (path == successor_for_bci(iter().next_bci()));
 
-  if (path_is_suitable_for_uncommon_trap(prob)) {
+  if (can_trap && path_is_suitable_for_uncommon_trap(prob)) {
     repush_if_args();
     Node* call = uncommon_trap(Deoptimization::Reason_unstable_if,
                   Deoptimization::Action_reinterpret,

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestAcmpWithUnstableIf.java

@@ -0,0 +1,145 @@
+/*
+ * Copyright (c) 2024, 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
+ * @key randomness
+ * @summary Test that deoptimization at unstable ifs in acmp works as expected.
+ * @library /test/lib
+ * @run main/othervm -XX:+EnableValhalla TestAcmpWithUnstableIf
+ * @run main/othervm -XX:+EnableValhalla -XX:CompileCommand=compileonly,TestAcmpWithUnstableIf::test* -Xbatch TestAcmpWithUnstableIf
+ */
+
+import jdk.test.lib.Asserts;
+import jdk.test.lib.Utils;
+
+public class TestAcmpWithUnstableIf {
+
+    public static final int     EQUAL = Utils.getRandomInstance().nextInt();
+    public static final int NOT_EQUAL = Utils.getRandomInstance().nextInt();
+
+    static value class MyValue {
+        int x;
+
+        public MyValue(int x) {
+            this.x = x;
+        }
+    }
+
+    public static int test1(MyValue val1, MyValue val2, int resEqual, int resNotEqual) {
+        if (val1 == val2) {
+            return resEqual;
+        }
+        return resNotEqual;
+    }
+
+    public static int test2(MyValue val1, MyValue val2, int resEqual, int resNotEqual) {
+        if (val1 == val2) {
+            return resEqual;
+        }
+        return resNotEqual;
+    }
+
+    public static int test3(MyValue val1, MyValue val2, int resEqual, int resNotEqual) {
+        if (val1 == val2) {
+            return resEqual;
+        }
+        return resNotEqual;
+    }
+
+    public static int test4(MyValue val1, MyValue val2, int resEqual, int resNotEqual) {
+        if (val1 == val2) {
+            return resEqual;
+        }
+        return resNotEqual;
+    }
+
+    public static int test5(MyValue val1, MyValue val2, int resEqual, int resNotEqual) {
+        if (val1 != val2) {
+            return resNotEqual;
+        }
+        return resEqual;
+    }
+
+    public static int test6(MyValue val1, MyValue val2, int resEqual, int resNotEqual) {
+        if (val1 != val2) {
+            return resNotEqual;
+        }
+        return resEqual;
+    }
+
+    public static int test7(MyValue val1, MyValue val2, int resEqual, int resNotEqual) {
+        if (val1 != val2) {
+            return resNotEqual;
+        }
+        return resEqual;
+    }
+
+    public static int test8(MyValue val1, MyValue val2, int resEqual, int resNotEqual) {
+        if (val1 != val2) {
+            return resNotEqual;
+        }
+        return resEqual;
+    }
+
+    public static void main(String[] args) {
+        MyValue val = new MyValue(EQUAL);
+        MyValue val_copy = new MyValue(EQUAL);
+        MyValue val_diff = new MyValue(EQUAL + 1);
+
+        // Warmup
+        for (int i = 0; i < 50_000; ++i) {
+            // Equal arguments, same oop
+            Asserts.assertEquals(test1(val, val, EQUAL, NOT_EQUAL), EQUAL);
+            Asserts.assertEquals(test5(val, val, EQUAL, NOT_EQUAL), EQUAL);
+
+            // Equal arguments, different oop
+            Asserts.assertEquals(test2(val, val_copy, EQUAL, NOT_EQUAL), EQUAL);
+            Asserts.assertEquals(test6(val, val_copy, EQUAL, NOT_EQUAL), EQUAL);
+
+            // Different arguments
+            Asserts.assertEquals(test3(val, val_diff, EQUAL, NOT_EQUAL), NOT_EQUAL);
+            Asserts.assertEquals(test4(val, val_diff, EQUAL, NOT_EQUAL), NOT_EQUAL);
+
+            Asserts.assertEquals(test7(val, val_diff, EQUAL, NOT_EQUAL), NOT_EQUAL);
+            Asserts.assertEquals(test8(val, val_diff, EQUAL, NOT_EQUAL), NOT_EQUAL);
+        }
+
+        // Now trigger deoptimization
+
+        // Different arguments
+        Asserts.assertEquals(test1(val, val_diff, EQUAL, NOT_EQUAL), NOT_EQUAL);
+        Asserts.assertEquals(test2(val, val_diff, EQUAL, NOT_EQUAL), NOT_EQUAL);
+
+        Asserts.assertEquals(test5(val, val_diff, EQUAL, NOT_EQUAL), NOT_EQUAL);
+        Asserts.assertEquals(test6(val, val_diff, EQUAL, NOT_EQUAL), NOT_EQUAL);
+
+        // Equal arguments, same oop
+        Asserts.assertEquals(test3(val, val, EQUAL, NOT_EQUAL), EQUAL);
+        Asserts.assertEquals(test7(val, val, EQUAL, NOT_EQUAL), EQUAL);
+
+        // Equal arguments, different oop
+        Asserts.assertEquals(test4(val, val_copy, EQUAL, NOT_EQUAL), EQUAL);
+        Asserts.assertEquals(test8(val, val_copy, EQUAL, NOT_EQUAL), EQUAL);
+    }
+}

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestLWorldProfiling.java

@@ -586,7 +586,7 @@ public void test20_verifier() {
 
     // branch frequency profiling causes not equal branch to be optimized out
     @Test
-    @IR(failOn = {SUBSTITUTABILITY_TEST})
+    @IR(counts = {IRNode.UNSTABLE_IF_TRAP, " = 1"})
     public boolean test21(Object o1, Object o2) {
         return o1 == o2;
     }
@@ -1036,9 +1036,9 @@ public void test37_verifier(RunInfo info) {
     @Test
     @IR(applyIfOr = {"UseACmpProfile", "true", "TypeProfileLevel", "= 222"},
         failOn = {SUBSTITUTABILITY_TEST},
-        counts = {CLASS_CHECK_TRAP, "= 1"})
+        counts = {CLASS_CHECK_TRAP, "= 2"})
     @IR(applyIfAnd = {"UseACmpProfile", "false", "TypeProfileLevel", "!= 222"},
-        counts = {SUBSTITUTABILITY_TEST, "= 1"})
+        counts = {SUBSTITUTABILITY_TEST, "= 2"})
     public void test38(Object o1, Object o2, Object o3) {
         if (o1 == o2) {
             test38_helper2();
@@ -1061,13 +1061,12 @@ public void test38_verifier() {
         test38_helper(testValue1, testValue2);
     }
 
-
     @Test
     @IR(applyIfOr = {"UseACmpProfile", "true", "TypeProfileLevel", "= 222"},
         failOn = {SUBSTITUTABILITY_TEST},
-        counts = {CLASS_CHECK_TRAP, "= 1"})
+        counts = {CLASS_CHECK_TRAP, "= 2"})
     @IR(applyIfAnd = {"UseACmpProfile", "false", "TypeProfileLevel", "!= 222"},
-        counts = {SUBSTITUTABILITY_TEST, "= 1"})
+        counts = {SUBSTITUTABILITY_TEST, "= 2"})
     public void test39(Object o1, Object o2, Object o3) {
         if (o1 == o2) {
             test39_helper2();

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestMethodHandles.java

@@ -148,6 +148,7 @@ public static void main(String[] args) {
 
         InlineTypes.getFramework()
                    .addScenarios(scenarios)
+                   .addFlags("-XX:CompileCommand=inline,java.lang.invoke.MethodHandleImpl::*")
                    .addHelperClasses(MyValue1.class,
                                      MyValue2.class,
                                      MyValue2Inline.class,