8255230: [lworld] C2 compilation fails with assert(!inline_alloc) failed: Inline type allocations should not have safepoint uses

Author: TobiHartmann

src/hotspot/share/opto/compile.cpp

@@ -1933,23 +1933,26 @@ static bool return_val_keeps_allocations_alive(Node* ret_val) {
   return some_allocations;
 }
 
-void Compile::process_inline_types(PhaseIterGVN &igvn, bool post_ea) {
+void Compile::process_inline_types(PhaseIterGVN &igvn, bool remove) {
   // Make inline types scalar in safepoints
   for (int i = _inline_type_nodes->length()-1; i >= 0; i--) {
     InlineTypeBaseNode* vt = _inline_type_nodes->at(i)->as_InlineTypeBase();
     vt->make_scalar_in_safepoints(&igvn);
   }
-  // Remove InlineTypePtr nodes only after EA to give scalar replacement a chance
-  // to remove buffer allocations. InlineType nodes are kept until loop opts and
-  // removed via InlineTypeNode::remove_redundant_allocations.
-  if (post_ea) {
+  if (remove) {
+    // Remove inline type nodes
     while (_inline_type_nodes->length() > 0) {
       InlineTypeBaseNode* vt = _inline_type_nodes->pop()->as_InlineTypeBase();
-      if (vt->is_InlineTypePtr()) {
+      if (vt->outcnt() == 0) {
+        igvn.remove_dead_node(vt);
+      } else if (vt->is_InlineTypePtr()) {
         igvn.replace_node(vt, vt->get_oop());
+      } else {
+        igvn.replace_node(vt, igvn.C->top());
       }
     }
   }
+  // TODO only check once we are removing, right?
   // Make sure that the return value does not keep an unused allocation alive
   if (tf()->returns_inline_type_as_fields()) {
     Node* ret = NULL;
@@ -2570,11 +2573,6 @@ void Compile::Optimize() {
     }
   }
 
-  if (_inline_type_nodes->length() > 0) {
-    // Process inline types again now that EA might have simplified the graph
-    process_inline_types(igvn, /* post_ea= */ true);
-  }
-
   // Loop transforms on the ideal graph.  Range Check Elimination,
   // peeling, unrolling, etc.
 
@@ -2659,6 +2657,12 @@ void Compile::Optimize() {
   bs->verify_gc_barriers(this, BarrierSetC2::BeforeMacroExpand);
 #endif
 
+  if (_inline_type_nodes->length() > 0) {
+    // Process inline type nodes again and remove them. From here
+    // on we don't need to keep track of field values anymore.
+    process_inline_types(igvn, /* remove= */ true);
+  }
+
   {
     TracePhase tp("macroExpand", &timers[_t_macroExpand]);
     PhaseMacroExpand  mex(igvn);

src/hotspot/share/opto/compile.hpp

@@ -724,7 +724,7 @@ class Compile : public Phase {
   // Keep track of inline type nodes for later processing
   void add_inline_type(Node* n);
   void remove_inline_type(Node* n);
-  void process_inline_types(PhaseIterGVN &igvn, bool post_ea = false);
+  void process_inline_types(PhaseIterGVN &igvn, bool remove = false);
 
   void adjust_flattened_array_access_aliases(PhaseIterGVN& igvn);
 

src/hotspot/share/opto/inlinetypenode.cpp

@@ -860,7 +860,6 @@ Node* InlineTypeNode::Ideal(PhaseGVN* phase, bool can_reshape) {
 }
 
 // Search for multiple allocations of this inline type and try to replace them by dominating allocations.
-// Then unlink the inline type node and remove it.
 void InlineTypeNode::remove_redundant_allocations(PhaseIterGVN* igvn, PhaseIdealLoop* phase) {
   // Search for allocations of this inline type. Ignore scalar replaceable ones, they
   // will be removed anyway and changing the memory chain will confuse other optimizations.
@@ -898,23 +897,14 @@ void InlineTypeNode::remove_redundant_allocations(PhaseIterGVN* igvn, PhaseIdeal
   for (DUIterator_Fast imax, i = fast_outs(imax); i < imax; i++) {
     Node* out = fast_out(i);
     if (out->is_InlineType()) {
-      // Unlink and recursively process inline type users
+      // Recursively process inline type users
       igvn->rehash_node_delayed(out);
-      int nb = out->replace_edge(this, igvn->C->top());
       out->as_InlineType()->remove_redundant_allocations(igvn, phase);
-      --i; imax -= nb;
     } else if (out->isa_Allocate() != NULL) {
       // Unlink AllocateNode
       assert(out->in(AllocateNode::InlineTypeNode) == this, "should be linked");
       igvn->replace_input_of(out, AllocateNode::InlineTypeNode, igvn->C->top());
       --i; --imax;
-    } else {
-#ifdef ASSERT
-      // The inline type should not have any other users at this time
-      out->dump();
-      assert(false, "unexpected user of inline type");
-#endif
     }
   }
-  igvn->remove_dead_node(this);
 }

src/hotspot/share/opto/loopopts.cpp

@@ -1618,7 +1618,6 @@ void PhaseIdealLoop::split_if_with_blocks_post(Node *n) {
   // Remove multiple allocations of the same inline type
   if (n->is_InlineType()) {
     n->as_InlineType()->remove_redundant_allocations(&_igvn, this);
-    return; // n is now dead
   }
 
   // Check for Opaque2's who's loop has disappeared - who's input is in the

src/hotspot/share/opto/split_if.cpp

@@ -239,15 +239,6 @@ bool PhaseIdealLoop::split_up( Node *n, Node *blk1, Node *blk2 ) {
     rtype = TypeLong::INT;
   }
 
-  // Inline types should not be split through Phis but each value input
-  // needs to be merged individually. At this point, inline types should
-  // only be used by AllocateNodes. Try to remove redundant allocations
-  // and unlink the now dead inline type node.
-  if (n->is_InlineType()) {
-    n->as_InlineType()->remove_redundant_allocations(&_igvn, this);
-    return true; // n is now dead
-  }
-
   // Now actually split-up this guy.  One copy per control path merging.
   Node *phi = PhiNode::make_blank(blk1, n);
   for( uint j = 1; j < blk1->req(); j++ ) {

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

@@ -3410,4 +3410,124 @@ public void test125_verifier(boolean warmup) {
         test125(testValue1);
         test125(null);
     }
+
+    // Test inline type that can only be scalarized after loop opts
+    @Test(failOn = ALLOC + LOAD + STORE)
+    @Warmup(10000)
+    public long test126(boolean trap) {
+        MyValue2 nonNull = MyValue2.createWithFieldsInline(rI, rD);
+        MyValue2.ref val = null;
+
+        for (int i = 0; i < 4; i++) {
+            if ((i % 2) == 0) {
+                val = nonNull;
+            }
+        }
+        // 'val' is always non-null here but that's only known after loop opts
+        if (trap) {
+            // Uncommon trap with an inline input that can only be scalarized after loop opts
+            return val.hash();
+        }
+        return 0;
+    }
+
+    @DontCompile
+    public void test126_verifier(boolean warmup) {
+        long res = test126(false);
+        Asserts.assertEquals(res, 0L);
+        if (!warmup) {
+            res = test126(true);
+            Asserts.assertEquals(res, testValue2.hash());
+        }
+    }
+
+    // Same as test126 but with interface type
+    @Test(failOn = ALLOC + LOAD + STORE)
+    @Warmup(10000)
+    public long test127(boolean trap) {
+        MyValue2 nonNull = MyValue2.createWithFieldsInline(rI, rD);
+        MyInterface val = null;
+
+        for (int i = 0; i < 4; i++) {
+            if ((i % 2) == 0) {
+                val = nonNull;
+            }
+        }
+        // 'val' is always non-null here but that's only known after loop opts
+        if (trap) {
+            // Uncommon trap with an inline input that can only be scalarized after loop opts
+            return val.hash();
+        }
+        return 0;
+    }
+
+    @DontCompile
+    public void test127_verifier(boolean warmup) {
+        long res = test127(false);
+        Asserts.assertEquals(res, 0L);
+        if (!warmup) {
+            res = test127(true);
+            Asserts.assertEquals(res, testValue2.hash());
+        }
+    }
+
+    // Test inline type that can only be scalarized after CCP
+    @Test(failOn = ALLOC + LOAD + STORE)
+    @Warmup(10000)
+    public long test128(boolean trap) {
+        MyValue2 nonNull = MyValue2.createWithFieldsInline(rI, rD);
+        MyValue2.ref val = null;
+
+        int limit = 2;
+        for (; limit < 4; limit *= 2);
+        for (int i = 2; i < limit; i++) {
+            val = nonNull;
+        }
+        // 'val' is always non-null here but that's only known after CCP
+        if (trap) {
+            // Uncommon trap with an inline input that can only be scalarized after CCP
+            return val.hash();
+        }
+        return 0;
+    }
+
+    @DontCompile
+    public void test128_verifier(boolean warmup) {
+        long res = test128(false);
+        Asserts.assertEquals(res, 0L);
+        if (!warmup) {
+            res = test128(true);
+            Asserts.assertEquals(res, testValue2.hash());
+        }
+    }
+
+    // Same as test128 but with interface type
+    @Test(failOn = ALLOC + LOAD + STORE)
+    @Warmup(10000)
+    public long test129(boolean trap) {
+        MyValue2 nonNull = MyValue2.createWithFieldsInline(rI, rD);
+        MyInterface val = null;
+
+        int limit = 2;
+        for (; limit < 4; limit *= 2);
+        for (int i = 0; i < limit; i++) {
+            val = nonNull;
+        }
+        // 'val' is always non-null here but that's only known after CCP
+        if (trap) {
+            // Uncommon trap with an inline input that can only be scalarized after CCP
+            return val.hash();
+        }
+        return 0;
+    }
+
+    @DontCompile
+    public void test129_verifier(boolean warmup) {
+        long res = test129(false);
+        Asserts.assertEquals(res, 0L);
+        if (!warmup) {
+            res = test129(true);
+            Asserts.assertEquals(res, testValue2.hash());
+        }
+    }
 }