8263227: C2: inconsistent spilling due to dead nodes in exception block

Eliminate dead nodes created by call-catch cleanup even if they have multiple
projections. Assert that definitions dominate uses and projections are scheduled
next to their parent nodes.

Reviewed-by: roland
Backport-of: d81b046

Author: TheRealMDoerr

src/hotspot/share/opto/block.cpp

@@ -1218,12 +1218,18 @@ void PhaseCFG::verify() const {
       for (uint k = 0; k < n->req(); k++) {
         Node *def = n->in(k);
         if (def && def != n) {
-          assert(get_block_for_node(def) || def->is_Con(), "must have block; constants for debug info ok");
-          // Verify that instructions in the block is in correct order.
+          Block* def_block = get_block_for_node(def);
+          assert(def_block || def->is_Con(), "must have block; constants for debug info ok");
+          // Verify that all definitions dominate their uses (except for virtual
+          // instructions merging multiple definitions).
+          assert(n->is_Root() || n->is_Region() || n->is_Phi() || n->is_MachMerge() ||
+                 def_block->dominates(block),
+                 "uses must be dominated by definitions");
+          // Verify that instructions in the block are in correct order.
           // Uses must follow their definition if they are at the same block.
           // Mostly done to check that MachSpillCopy nodes are placed correctly
           // when CreateEx node is moved in build_ifg_physical().
-          if (get_block_for_node(def) == block && !(block->head()->is_Loop() && n->is_Phi()) &&
+          if (def_block == block && !(block->head()->is_Loop() && n->is_Phi()) &&
               // See (+++) comment in reg_split.cpp
               !(n->jvms() != NULL && n->jvms()->is_monitor_use(k))) {
             bool is_loop = false;
@@ -1239,6 +1245,14 @@ void PhaseCFG::verify() const {
           }
         }
       }
+      if (n->is_Proj()) {
+        assert(j >= 1, "a projection cannot be the first instruction in a block");
+        Node* pred = block->get_node(j - 1);
+        Node* parent = n->in(0);
+        assert(parent != NULL, "projections must have a parent");
+        assert(pred == parent || (pred->is_Proj() && pred->in(0) == parent),
+               "projections must follow their parents or other sibling projections");
+      }
     }
 
     j = block->end_idx();

src/hotspot/share/opto/lcm.cpp

@@ -1390,14 +1390,40 @@ void PhaseCFG::call_catch_cleanup(Block* block) {
   }
 
   // If the successor blocks have a CreateEx node, move it back to the top
-  for(uint i4 = 0; i4 < block->_num_succs; i4++ ) {
+  for (uint i4 = 0; i4 < block->_num_succs; i4++) {
     Block *sb = block->_succs[i4];
     uint new_cnt = end - beg;
-    // Remove any newly created, but dead, nodes.
-    for( uint j = new_cnt; j > 0; j-- ) {
+    // Remove any newly created, but dead, nodes by traversing their schedule
+    // backwards. Here, a dead node is a node whose only outputs (if any) are
+    // unused projections.
+    for (uint j = new_cnt; j > 0; j--) {
       Node *n = sb->get_node(j);
-      if (n->outcnt() == 0 &&
-          (!n->is_Proj() || n->as_Proj()->in(0)->outcnt() == 1) ){
+      // Individual projections are examined together with all siblings when
+      // their parent is visited.
+      if (n->is_Proj()) {
+        continue;
+      }
+      bool dead = true;
+      for (DUIterator_Fast imax, i = n->fast_outs(imax); i < imax; i++) {
+        Node* out = n->fast_out(i);
+        // n is live if it has a non-projection output or a used projection.
+        if (!out->is_Proj() || out->outcnt() > 0) {
+          dead = false;
+          break;
+        }
+      }
+      if (dead) {
+        // n's only outputs (if any) are unused projections scheduled next to n
+        // (see PhaseCFG::select()). Remove these projections backwards.
+        for (uint k = j + n->outcnt(); k > j; k--) {
+          Node* proj = sb->get_node(k);
+          assert(proj->is_Proj() && proj->in(0) == n,
+                 "projection should correspond to dead node");
+          proj->disconnect_inputs(NULL, C);
+          sb->remove_node(k);
+          new_cnt--;
+        }
+        // Now remove the node itself.
         n->disconnect_inputs(NULL, C);
         sb->remove_node(j);
         new_cnt--;

src/hotspot/share/opto/reg_split.cpp

@@ -114,6 +114,8 @@ void PhaseChaitin::insert_proj( Block *b, uint i, Node *spill, uint maxlrg ) {
   // Do not insert between a call and his Catch
   if( b->get_node(i)->is_Catch() ) {
     // Put the instruction at the top of the fall-thru block.
+    // This assumes that the instruction is not used in the other exception
+    // blocks. Global code motion is responsible for maintaining this invariant.
     // Find the fall-thru projection
     while( 1 ) {
       const CatchProjNode *cp = b->get_node(++i)->as_CatchProj();

test/hotspot/jtreg/compiler/exceptions/TestSpilling.java

@@ -0,0 +1,65 @@
+/*
+ * Copyright (c) 2021, 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.
+ */
+
+package compiler.exceptions;
+
+/**
+ * @test
+ * @bug 8263227
+ * @summary Tests that users of return values from exception-throwing method
+ *          calls are not duplicated in the call's exception path. The second
+ *          run with a variable seed is added for test robustness.
+ * @library /test/lib /
+ * @run main/othervm -XX:+IgnoreUnrecognizedVMOptions
+ *                   -XX:+UnlockDiagnosticVMOptions
+ *                   -Xbatch -XX:+StressGCM -XX:StressSeed=0
+ *                   -XX:+VerifyRegisterAllocator
+ *                   -XX:CompileCommand=dontinline,java.lang.Integer::*
+ *                   compiler.exceptions.TestSpilling
+ * @run main/othervm -XX:+IgnoreUnrecognizedVMOptions
+ *                   -XX:+UnlockDiagnosticVMOptions
+ *                   -Xbatch -XX:+StressGCM
+ *                   -XX:+VerifyRegisterAllocator
+ *                   -XX:CompileCommand=dontinline,java.lang.Integer::*
+ *                   compiler.exceptions.TestSpilling
+ */
+
+public class TestSpilling {
+
+    public static void test() {
+        int a = Integer.valueOf(42).intValue();
+        // After global code motion, the logic below should only be placed in
+        // the fall-through path of java.lang.Integer::intValue(). Otherwise,
+        // live range splitting might create uses without reaching definitions
+        // if 'a' is spilled.
+        int b = (((a & 0x0000F000)) + 1);
+        int c = a / b + ((a % b > 0) ? 1 : 0);
+    }
+
+    public static void main(String[] args) {
+        for (int i = 0; i < 10_000; i++) {
+            test();
+        }
+    }
+
+}