8356084: C2: Data is wrongly rewired to Initialized Assertion Predicates instead of Template Assertion Predicates

Reviewed-by: epeter, kvn

Author: chhagedorn

src/hotspot/share/opto/predicates.cpp

@@ -984,15 +984,16 @@ void CreateAssertionPredicatesVisitor::visit(const TemplateAssertionPredicate& t
 
 // Create an Initialized Assertion Predicate from the provided Template Assertion Predicate.
 InitializedAssertionPredicate CreateAssertionPredicatesVisitor::initialize_from_template(
-    const TemplateAssertionPredicate& template_assertion_predicate, Node* new_control) const {
+  const TemplateAssertionPredicate& template_assertion_predicate, IfTrueNode* cloned_template_predicate_tail) const {
   DEBUG_ONLY(template_assertion_predicate.verify();)
   IfNode* template_head = template_assertion_predicate.head();
   InitializedAssertionPredicateCreator initialized_assertion_predicate_creator(_phase);
   InitializedAssertionPredicate initialized_assertion_predicate =
-      initialized_assertion_predicate_creator.create_from_template(template_head, new_control, _init, _stride);
+      initialized_assertion_predicate_creator.create_from_template(template_head, cloned_template_predicate_tail, _init,
+                                                                   _stride);
 
   DEBUG_ONLY(initialized_assertion_predicate.verify();)
-  template_assertion_predicate.rewire_loop_data_dependencies(initialized_assertion_predicate.tail(),
+  template_assertion_predicate.rewire_loop_data_dependencies(cloned_template_predicate_tail,
                                                              _node_in_loop_body, _phase);
   rewire_to_old_predicate_chain_head(initialized_assertion_predicate.tail());
   return initialized_assertion_predicate;

src/hotspot/share/opto/predicates.hpp

@@ -1098,7 +1098,7 @@ class CreateAssertionPredicatesVisitor : public PredicateVisitor {
   clone_template_and_replace_init_input(const TemplateAssertionPredicate& template_assertion_predicate) const;
 
   InitializedAssertionPredicate initialize_from_template(const TemplateAssertionPredicate& template_assertion_predicate,
-                                                         Node* new_control) const;
+                                                         IfTrueNode* cloned_template_predicate_tail) const;
   void rewire_to_old_predicate_chain_head(Node* initialized_assertion_predicate_success_proj) const;
 
  public:

test/hotspot/jtreg/compiler/predicates/assertion/TestAssertionPredicates.java

@@ -179,6 +179,18 @@
  *                   compiler.predicates.assertion.TestAssertionPredicates Stress
  */
 
+/*
+ * @test id=StressXcompMaxUnroll0
+ * @key randomness
+ * @bug 8288981 8356084
+ * @requires vm.compiler2.enabled
+ * @run main/othervm -Xcomp -XX:+UnlockDiagnosticVMOptions -XX:+StressGCM -XX:+AbortVMOnCompilationFailure
+ *                   -XX:LoopMaxUnroll=0 -XX:+IgnoreUnrecognizedVMOptions -XX:-KillPathsReachableByDeadTypeNode
+ *                   -XX:CompileCommand=compileonly,compiler.predicates.assertion.TestAssertionPredicates::*
+ *                   -XX:CompileCommand=dontinline,compiler.predicates.assertion.TestAssertionPredicates::*
+ *                   compiler.predicates.assertion.TestAssertionPredicates StressXcompMaxUnroll0
+ */
+
 /*
  * @test id=NoLoopPredicationXcomp
  * @bug 8288981 8350577
@@ -226,6 +238,7 @@ public class TestAssertionPredicates {
     static boolean flagFalse, flagFalse2;
     static int iFld = 34;
     static int iFld2, iFld3;
+    static int two = 2;
     static long lFld, lFldOne = 1;
     static float fFld;
     static short sFld;
@@ -238,7 +251,7 @@ static class Foo {
     }
 
     static Foo foo = new Foo();
-
+    static int fooArrSize = 10000001;
 
     public static void main(String[] args) {
         switch (args[0]) {
@@ -315,14 +328,15 @@ public static void main(String[] args) {
                     test8308504No2();
                 }
             }
-            case "DataUpdate" -> {
+            case "DataUpdate", "StressXcompMaxUnroll0" -> {
                 for (int i = 0; i < 10; i++) {
                     // The following tests create large arrays. Limit the number of invocations to reduce the time spent.
                     flag = !flag;
                     testDataUpdateUnswitchingPeelingUnroll();
                     testDataUpdateUnswitchUnroll();
                     testDataUpdateUnroll();
                     testDataUpdatePeelingUnroll();
+                    testPeelingThreeTimesDataUpdate();
                 }
             }
             case "CloneDown" -> {
@@ -1064,18 +1078,18 @@ static void testDataUpdateUnswitchingPeelingUnroll() {
         for (int i = 0; i < limit; i++) {
             fooArr[10000000 * i] = foo;
         }
-        // 9) This loop is not optimized in any way because we have a call inside the loop. This loop is only required
+        // 10) This loop is not optimized in any way because we have a call inside the loop. This loop is only required
         //    to trigger a crash.
-        // 10) During GCM with StressGCM, we could schedule the LoadN from the main loop before checking if we should
+        // 11) During GCM with StressGCM, we could schedule the LoadN from the main loop before checking if we should
         //     enter the main loop. When 'flag' is true, we only have an array of size 20000001. We then perform
         //     the LoadN[3*10000000] and crash when the memory is unmapped.
         for (float f = 0; f < 1.6f; f += 0.5f) {
             // 2) Loop is unswitched
-            // 3) Both loop are peeled (we focus on one of those since both are almost identical except for the
+            // 4) Both loop are peeled (we focus on one of those since both are almost identical except for the
             //    unswitched condition):
             //      Peeled iteration [i = 0]
             //      Loop [i = 1..4, stride = 1]
-            // 4) Loop unroll policy now returns true.
+            // 5) Loop unroll policy now returns true.
             //    Peeled iteration [i = 0]
             //    - Loop is pre-main-posted
             //        Loop-pre[i = 1..4, stride = 1]
@@ -1085,16 +1099,16 @@ static void testDataUpdateUnswitchingPeelingUnroll() {
             //        Loop-pre[i = 1..4, stride = 1]
             //        Loop-main[i = 2..4, stride = 2]
             //        Loop-post[i = 2..4, stride = 1]
-            // 5) During IGVN, we find that the backedge is never taken for main loop (we would over-iteratre) and it
+            // 6) During IGVN, we find that the backedge is never taken for main loop (we would over-iteratre) and it
             //    collapses to a single iteration.
-            // 6) After loop opts, the pre-loop is removed.
+            // 7) After loop opts, the pre-loop is removed.
             for (int i = 0; i < limit; i++) {
                 // 1) Hoisted with a Hoisted Range Check Predicate
-                // 7) The 'i = 1' value is propagated to the single main loop iteration and we have the following
+                // 8) The 'i = 1' value is propagated to the single main loop iteration and we have the following
                 //    fixed-index accesses:
                 //      LoadN[2*10000000];
                 //      LoadN[3*10000000];
-                // 8) Without explicitly pinning the LoadN from the main loop at the main loop entry (i.e. below the
+                // 9) Without explicitly pinning the LoadN from the main loop at the main loop entry (i.e. below the
                 //    zero trip guard), they are still pinned below the Hoisted Range Check Predicate before the loop.
                 fooArr[i * 10000000].iFld += 34;
                 if (flagFalse) {
@@ -1327,11 +1341,74 @@ static void testDataUpdatePeelingUnroll() {
         }
     }
 
+
+    // -Xcomp -XX:LoopMaxUnroll=0 -XX:+StressGCM -XX:CompileCommand=compileonly,Test*::*
+    private static void testPeelingThreeTimesDataUpdate() {
+        Foo[] fooArr = new Foo[fooArrSize];
+        for (int i = 0; i < two; i++) {
+            fooArr[10000000 * i] = foo;
+        }
+        int x = 0;
+
+        // 2) The Hoisted Range Check Predicate is accompanied by two Template Assertion Predicates. The LoadN node,
+        //    previously pinned at the hoisted range check, is now pinned at the Template Assertion Predicate. Note
+        //    that the LoadN is still inside the loop body.
+        // 3) The loop is now peeled 3 times which also peels 3 loads from 'fooArr' out of the loop:
+        //       // Peeled section from 1st Loop Peeling
+        //       ...
+        //       LoadN[0]
+        //       ...
+        //       <loop entry guard>
+        //       // Peeled section from 2nd Loop Peeling
+        //       ...
+        //       LoadN[10000000]
+        //       Initialized Assertion Predicate (***)
+        //       <loop entry guard>
+        //       // Peeled section from 3rd Loop Peeling
+        //       ...
+        //       LoadN[20000000]
+        //       ...
+        //       Initialized Assertion Predicate
+        //       <loop entry guard>
+        //       Template Assertion Predicate
+        //       Initialized Assertion Predicate
+        // Loop:
+        //   LoadN[i*10000000]
+        //
+        // To avoid that the peeled LoadN nodes float above the corresponding loop entry guards, we need to pin them
+        // below. That is done by updating the dependency of the peeled LoadN to the new Template Assertion Predicate.
+        // This is currently broken: We wrongly set the dependency to the Initialized Assertion Predicate instead of the
+        // Template Assertion Predicate. We can then no longer find the dependency and miss to update it in the next
+        // Loop Peeling application. As a result, all the LoadN pile up at the originally added Initialized Assertion
+        // Predicate of the first Loop Peeling application at (***).
+        //
+        // With GCM, we could schedule LoadN[20000000] at (***), before the loop entry corresponding loop entry guard
+        // for this load. We then crash during runtime because we are accessing an out-of-range index. The fix is to
+        // properly update the data dependencies to the Template Assertion Predicates and not the Initialized Assertion
+        // Predicates.
+        for (int i = 0; i < two; i++) {
+            // 1) Hoisted with a Hoisted Range Check Predicate
+            x += fooArr[i * 10000000].iFld;
+
+            if (iFld2 == 4) {
+                return;
+            }
+
+            if (i > 0 && iFld2 == 3) {
+                iFld2 = 42;
+                return;
+            }
+
+            if (i > 1 && iFld2 == 2) {
+                return;
+            }
+        }
+    }
+
     /*
      * Tests collected in JBS and duplicated issues
      */
 
-
     // -Xbatch -XX:CompileCommand=compileonly,Test*::*
     static void test8305428() {
         int j = 1;