Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 10 commits into from
@@ -1245,12 +1245,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;
@@ -1266,6 +1272,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();
@@ -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(C);
sb->remove_node(k);
This conversation was marked as resolved by robcasloz

This comment has been minimized.

Loading
@vnkozlov

vnkozlov Apr 6, 2021
Contributor

If you remove node here then j could be incorrect in sb->remove_node(j) at line #1424

This comment has been minimized.

Loading
@robcasloz

robcasloz Apr 8, 2021
Author Contributor

k should always be greater than j, as sb->get_node(k) is a projection of sb->get_node(j). Shouldn't this make the removal safe?

This comment has been minimized.

Loading
@vnkozlov

vnkozlov Apr 8, 2021
Contributor

Yes, you are right - users should follow.

new_cnt--;
}
// Now remove the node itself.
n->disconnect_inputs(C);
sb->remove_node(j);
new_cnt--;
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2000, 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2000, 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
@@ -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();
@@ -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();
}
}

}