8069191: moving predicate out of loops may cause array accesses to bypass null check

Remove CastPP nodes only during final graph reshape

Reviewed-by: kvn, jrose
This commit is contained in:
Roland Westrelin 2015-03-24 10:25:09 +01:00
parent 51fd716978
commit 05ea4dbf1e
14 changed files with 197 additions and 295 deletions

View File

@ -73,16 +73,6 @@ Node *ConstraintCastNode::Ideal(PhaseGVN *phase, bool can_reshape){
return (in(0) && remove_dead_region(phase, can_reshape)) ? this : NULL;
}
//------------------------------Ideal_DU_postCCP-------------------------------
// Throw away cast after constant propagation
Node *ConstraintCastNode::Ideal_DU_postCCP( PhaseCCP *ccp ) {
const Type *t = ccp->type(in(1));
ccp->hash_delete(this);
set_type(t); // Turn into ID function
ccp->hash_insert(this);
return this;
}
uint CastIINode::size_of() const {
return sizeof(*this);
}
@ -164,13 +154,6 @@ const Type *CastIINode::Value(PhaseTransform *phase) const {
return res;
}
Node *CastIINode::Ideal_DU_postCCP(PhaseCCP *ccp) {
if (_carry_dependency) {
return NULL;
}
return ConstraintCastNode::Ideal_DU_postCCP(ccp);
}
#ifndef PRODUCT
void CastIINode::dump_spec(outputStream *st) const {
TypeNode::dump_spec(st);
@ -180,20 +163,6 @@ void CastIINode::dump_spec(outputStream *st) const {
}
#endif
//=============================================================================
//------------------------------Ideal_DU_postCCP-------------------------------
// If not converting int->oop, throw away cast after constant propagation
Node *CastPPNode::Ideal_DU_postCCP( PhaseCCP *ccp ) {
const Type *t = ccp->type(in(1));
if (!t->isa_oop_ptr() || ((in(1)->is_DecodeN()) && Matcher::gen_narrow_oop_implicit_null_checks())) {
return NULL; // do not transform raw pointers or narrow oops
}
return ConstraintCastNode::Ideal_DU_postCCP(ccp);
}
//=============================================================================
//------------------------------Identity---------------------------------------
// If input is already higher or equal to cast type, then this is an identity.

View File

@ -42,7 +42,6 @@ class ConstraintCastNode: public TypeNode {
virtual Node *Ideal(PhaseGVN *phase, bool can_reshape);
virtual int Opcode() const;
virtual uint ideal_reg() const = 0;
virtual Node *Ideal_DU_postCCP( PhaseCCP * );
};
//------------------------------CastIINode-------------------------------------
@ -63,7 +62,6 @@ class CastIINode: public ConstraintCastNode {
virtual uint ideal_reg() const { return Op_RegI; }
virtual Node *Identity( PhaseTransform *phase );
virtual const Type *Value( PhaseTransform *phase ) const;
virtual Node *Ideal_DU_postCCP( PhaseCCP * );
#ifndef PRODUCT
virtual void dump_spec(outputStream *st) const;
#endif
@ -76,7 +74,6 @@ class CastPPNode: public ConstraintCastNode {
CastPPNode (Node *n, const Type *t ): ConstraintCastNode(n, t) {}
virtual int Opcode() const;
virtual uint ideal_reg() const { return Op_RegP; }
virtual Node *Ideal_DU_postCCP( PhaseCCP * );
};
//------------------------------CheckCastPPNode--------------------------------
@ -94,9 +91,6 @@ class CheckCastPPNode: public TypeNode {
virtual Node *Ideal(PhaseGVN *phase, bool can_reshape);
virtual int Opcode() const;
virtual uint ideal_reg() const { return Op_RegP; }
// No longer remove CheckCast after CCP as it gives me a place to hang
// the proper address type - which is required to compute anti-deps.
//virtual Node *Ideal_DU_postCCP( PhaseCCP * );
};

View File

@ -2811,9 +2811,38 @@ void Compile::final_graph_reshaping_impl( Node *n, Final_Reshape_Counts &frc) {
break;
}
#ifdef _LP64
case Op_CastPP:
if (n->in(1)->is_DecodeN() && Matcher::gen_narrow_oop_implicit_null_checks()) {
case Op_CastPP: {
// Remove CastPP nodes to gain more freedom during scheduling but
// keep the dependency they encode as control or precedence edges
// (if control is set already) on memory operations. Some CastPP
// nodes don't have a control (don't carry a dependency): skip
// those.
if (n->in(0) != NULL) {
ResourceMark rm;
Unique_Node_List wq;
wq.push(n);
for (uint next = 0; next < wq.size(); ++next) {
Node *m = wq.at(next);
for (DUIterator_Fast imax, i = m->fast_outs(imax); i < imax; i++) {
Node* use = m->fast_out(i);
if (use->is_Mem() || use->is_EncodeNarrowPtr()) {
use->ensure_control_or_add_prec(n->in(0));
} else if (use->in(0) == NULL) {
switch(use->Opcode()) {
case Op_AddP:
case Op_DecodeN:
case Op_DecodeNKlass:
case Op_CheckCastPP:
case Op_CastPP:
wq.push(use);
break;
}
}
}
}
}
const bool is_LP64 = LP64_ONLY(true) NOT_LP64(false);
if (is_LP64 && n->in(1)->is_DecodeN() && Matcher::gen_narrow_oop_implicit_null_checks()) {
Node* in1 = n->in(1);
const Type* t = n->bottom_type();
Node* new_in1 = in1->clone();
@ -2846,9 +2875,15 @@ void Compile::final_graph_reshaping_impl( Node *n, Final_Reshape_Counts &frc) {
if (in1->outcnt() == 0) {
in1->disconnect_inputs(NULL, this);
}
} else {
n->subsume_by(n->in(1), this);
if (n->outcnt() == 0) {
n->disconnect_inputs(NULL, this);
}
}
break;
}
#ifdef _LP64
case Op_CmpP:
// Do this transformation here to preserve CmpPNode::sub() and
// other TypePtr related Ideal optimizations (for example, ptr nullness).

View File

@ -100,6 +100,9 @@ void PhaseCFG::replace_block_proj_ctrl( Node *n ) {
}
}
static bool is_dominator(Block* d, Block* n) {
return d->dom_lca(n) == d;
}
//------------------------------schedule_pinned_nodes--------------------------
// Set the basic block for Nodes pinned into blocks
@ -122,6 +125,42 @@ void PhaseCFG::schedule_pinned_nodes(VectorSet &visited) {
schedule_node_into_block(node, block);
}
// If the node has precedence edges (added when CastPP nodes are
// removed in final_graph_reshaping), fix the control of the
// node to cover the precedence edges and remove the
// dependencies.
Node* n = NULL;
for (uint i = node->len()-1; i >= node->req(); i--) {
Node* m = node->in(i);
if (m == NULL) continue;
// Skip the precedence edge if the test that guarded a CastPP:
// - was optimized out during escape analysis
// (OptimizePtrCompare): the CastPP's control isn't an end of
// block.
// - is moved in the branch of a dominating If: the control of
// the CastPP is then a Region.
if (m->is_block_proj() || m->is_block_start()) {
node->rm_prec(i);
if (n == NULL) {
n = m;
} else {
Block* bn = get_block_for_node(n);
Block* bm = get_block_for_node(m);
assert(is_dominator(bn, bm) || is_dominator(bm, bn), "one must dominate the other");
n = is_dominator(bn, bm) ? m : n;
}
}
}
if (n != NULL) {
assert(node->in(0), "control should have been set");
Block* bn = get_block_for_node(n);
Block* bnode = get_block_for_node(node->in(0));
assert(is_dominator(bn, bnode) || is_dominator(bnode, bn), "one must dominate the other");
if (!is_dominator(bn, bnode)) {
node->set_req(0, n);
}
}
// process all inputs that are non NULL
for (int i = node->req() - 1; i >= 0; --i) {
if (node->in(i) != NULL) {

View File

@ -1049,6 +1049,15 @@ Node *Matcher::xform( Node *n, int max_stack ) {
mstack.push(m, Visit, n, -1);
}
// Handle precedence edges for interior nodes
for (i = n->len()-1; (uint)i >= n->req(); i--) {
Node *m = n->in(i);
if (m == NULL || C->node_arena()->contains(m)) continue;
n->rm_prec(i);
// set -1 to call add_prec() instead of set_req() during Step1
mstack.push(m, Visit, n, -1);
}
// For constant debug info, I'd rather have unmatched constants.
int cnt = n->req();
JVMState* jvms = n->jvms();
@ -1738,6 +1747,14 @@ MachNode *Matcher::ReduceInst( State *s, int rule, Node *&mem ) {
return ex;
}
void Matcher::handle_precedence_edges(Node* n, MachNode *mach) {
for (uint i = n->req(); i < n->len(); i++) {
if (n->in(i) != NULL) {
mach->add_prec(n->in(i));
}
}
}
void Matcher::ReduceInst_Chain_Rule( State *s, int rule, Node *&mem, MachNode *mach ) {
// 'op' is what I am expecting to receive
int op = _leftOp[rule];
@ -1772,6 +1789,8 @@ void Matcher::ReduceInst_Chain_Rule( State *s, int rule, Node *&mem, MachNode *m
uint Matcher::ReduceInst_Interior( State *s, int rule, Node *&mem, MachNode *mach, uint num_opnds ) {
handle_precedence_edges(s->_leaf, mach);
if( s->_leaf->is_Load() ) {
Node *mem2 = s->_leaf->in(MemNode::Memory);
assert( mem == (Node*)1 || mem == mem2, "multiple Memories being matched at once?" );
@ -1854,6 +1873,9 @@ void Matcher::ReduceOper( State *s, int rule, Node *&mem, MachNode *mach ) {
mem = s->_leaf->in(MemNode::Memory);
debug_only(_mem_node = s->_leaf;)
}
handle_precedence_edges(s->_leaf, mach);
if( s->_leaf->in(0) && s->_leaf->req() > 1) {
if( !mach->in(0) )
mach->set_req(0,s->_leaf->in(0));

View File

@ -124,6 +124,8 @@ class Matcher : public PhaseTransform {
// Mach node for ConP #NULL
MachNode* _mach_null;
void handle_precedence_edges(Node* n, MachNode *mach);
public:
int LabelRootDepth;
// Convert ideal machine register to a register mask for spill-loads

View File

@ -652,216 +652,6 @@ const TypePtr* MemNode::calculate_adr_type(const Type* t, const TypePtr* cross_c
}
}
//------------------------adr_phi_is_loop_invariant----------------------------
// A helper function for Ideal_DU_postCCP to check if a Phi in a counted
// loop is loop invariant. Make a quick traversal of Phi and associated
// CastPP nodes, looking to see if they are a closed group within the loop.
bool MemNode::adr_phi_is_loop_invariant(Node* adr_phi, Node* cast) {
// The idea is that the phi-nest must boil down to only CastPP nodes
// with the same data. This implies that any path into the loop already
// includes such a CastPP, and so the original cast, whatever its input,
// must be covered by an equivalent cast, with an earlier control input.
ResourceMark rm;
// The loop entry input of the phi should be the unique dominating
// node for every Phi/CastPP in the loop.
Unique_Node_List closure;
closure.push(adr_phi->in(LoopNode::EntryControl));
// Add the phi node and the cast to the worklist.
Unique_Node_List worklist;
worklist.push(adr_phi);
if( cast != NULL ){
if( !cast->is_ConstraintCast() ) return false;
worklist.push(cast);
}
// Begin recursive walk of phi nodes.
while( worklist.size() ){
// Take a node off the worklist
Node *n = worklist.pop();
if( !closure.member(n) ){
// Add it to the closure.
closure.push(n);
// Make a sanity check to ensure we don't waste too much time here.
if( closure.size() > 20) return false;
// This node is OK if:
// - it is a cast of an identical value
// - or it is a phi node (then we add its inputs to the worklist)
// Otherwise, the node is not OK, and we presume the cast is not invariant
if( n->is_ConstraintCast() ){
worklist.push(n->in(1));
} else if( n->is_Phi() ) {
for( uint i = 1; i < n->req(); i++ ) {
worklist.push(n->in(i));
}
} else {
return false;
}
}
}
// Quit when the worklist is empty, and we've found no offending nodes.
return true;
}
//------------------------------Ideal_DU_postCCP-------------------------------
// Find any cast-away of null-ness and keep its control. Null cast-aways are
// going away in this pass and we need to make this memory op depend on the
// gating null check.
Node *MemNode::Ideal_DU_postCCP( PhaseCCP *ccp ) {
return Ideal_common_DU_postCCP(ccp, this, in(MemNode::Address));
}
// I tried to leave the CastPP's in. This makes the graph more accurate in
// some sense; we get to keep around the knowledge that an oop is not-null
// after some test. Alas, the CastPP's interfere with GVN (some values are
// the regular oop, some are the CastPP of the oop, all merge at Phi's which
// cannot collapse, etc). This cost us 10% on SpecJVM, even when I removed
// some of the more trivial cases in the optimizer. Removing more useless
// Phi's started allowing Loads to illegally float above null checks. I gave
// up on this approach. CNC 10/20/2000
// This static method may be called not from MemNode (EncodePNode calls it).
// Only the control edge of the node 'n' might be updated.
Node *MemNode::Ideal_common_DU_postCCP( PhaseCCP *ccp, Node* n, Node* adr ) {
Node *skipped_cast = NULL;
// Need a null check? Regular static accesses do not because they are
// from constant addresses. Array ops are gated by the range check (which
// always includes a NULL check). Just check field ops.
if( n->in(MemNode::Control) == NULL ) {
// Scan upwards for the highest location we can place this memory op.
while( true ) {
switch( adr->Opcode() ) {
case Op_AddP: // No change to NULL-ness, so peek thru AddP's
adr = adr->in(AddPNode::Base);
continue;
case Op_DecodeN: // No change to NULL-ness, so peek thru
case Op_DecodeNKlass:
adr = adr->in(1);
continue;
case Op_EncodeP:
case Op_EncodePKlass:
// EncodeP node's control edge could be set by this method
// when EncodeP node depends on CastPP node.
//
// Use its control edge for memory op because EncodeP may go away
// later when it is folded with following or preceding DecodeN node.
if (adr->in(0) == NULL) {
// Keep looking for cast nodes.
adr = adr->in(1);
continue;
}
ccp->hash_delete(n);
n->set_req(MemNode::Control, adr->in(0));
ccp->hash_insert(n);
return n;
case Op_CastPP:
// If the CastPP is useless, just peek on through it.
if( ccp->type(adr) == ccp->type(adr->in(1)) ) {
// Remember the cast that we've peeked though. If we peek
// through more than one, then we end up remembering the highest
// one, that is, if in a loop, the one closest to the top.
skipped_cast = adr;
adr = adr->in(1);
continue;
}
// CastPP is going away in this pass! We need this memory op to be
// control-dependent on the test that is guarding the CastPP.
ccp->hash_delete(n);
n->set_req(MemNode::Control, adr->in(0));
ccp->hash_insert(n);
return n;
case Op_Phi:
// Attempt to float above a Phi to some dominating point.
if (adr->in(0) != NULL && adr->in(0)->is_CountedLoop()) {
// If we've already peeked through a Cast (which could have set the
// control), we can't float above a Phi, because the skipped Cast
// may not be loop invariant.
if (adr_phi_is_loop_invariant(adr, skipped_cast)) {
adr = adr->in(1);
continue;
}
}
// Intentional fallthrough!
// No obvious dominating point. The mem op is pinned below the Phi
// by the Phi itself. If the Phi goes away (no true value is merged)
// then the mem op can float, but not indefinitely. It must be pinned
// behind the controls leading to the Phi.
case Op_CheckCastPP:
// These usually stick around to change address type, however a
// useless one can be elided and we still need to pick up a control edge
if (adr->in(0) == NULL) {
// This CheckCastPP node has NO control and is likely useless. But we
// need check further up the ancestor chain for a control input to keep
// the node in place. 4959717.
skipped_cast = adr;
adr = adr->in(1);
continue;
}
ccp->hash_delete(n);
n->set_req(MemNode::Control, adr->in(0));
ccp->hash_insert(n);
return n;
// List of "safe" opcodes; those that implicitly block the memory
// op below any null check.
case Op_CastX2P: // no null checks on native pointers
case Op_Parm: // 'this' pointer is not null
case Op_LoadP: // Loading from within a klass
case Op_LoadN: // Loading from within a klass
case Op_LoadKlass: // Loading from within a klass
case Op_LoadNKlass: // Loading from within a klass
case Op_ConP: // Loading from a klass
case Op_ConN: // Loading from a klass
case Op_ConNKlass: // Loading from a klass
case Op_CreateEx: // Sucking up the guts of an exception oop
case Op_Con: // Reading from TLS
case Op_CMoveP: // CMoveP is pinned
case Op_CMoveN: // CMoveN is pinned
break; // No progress
case Op_Proj: // Direct call to an allocation routine
case Op_SCMemProj: // Memory state from store conditional ops
#ifdef ASSERT
{
assert(adr->as_Proj()->_con == TypeFunc::Parms, "must be return value");
const Node* call = adr->in(0);
if (call->is_CallJava()) {
const CallJavaNode* call_java = call->as_CallJava();
const TypeTuple *r = call_java->tf()->range();
assert(r->cnt() > TypeFunc::Parms, "must return value");
const Type* ret_type = r->field_at(TypeFunc::Parms);
assert(ret_type && ret_type->isa_ptr(), "must return pointer");
// We further presume that this is one of
// new_instance_Java, new_array_Java, or
// the like, but do not assert for this.
} else if (call->is_Allocate()) {
// similar case to new_instance_Java, etc.
} else if (!call->is_CallLeaf()) {
// Projections from fetch_oop (OSR) are allowed as well.
ShouldNotReachHere();
}
}
#endif
break;
default:
ShouldNotReachHere();
}
break;
}
}
return NULL; // No progress
}
//=============================================================================
// Should LoadNode::Ideal() attempt to remove control edges?
bool LoadNode::can_remove_control() const {

View File

@ -84,10 +84,6 @@ public:
// This one should probably be a phase-specific function:
static bool all_controls_dominate(Node* dom, Node* sub);
// Find any cast-away of null-ness and keep its control.
static Node *Ideal_common_DU_postCCP( PhaseCCP *ccp, Node* n, Node* adr );
virtual Node *Ideal_DU_postCCP( PhaseCCP *ccp );
virtual const class TypePtr *adr_type() const; // returns bottom_type of address
// Shared code for Ideal methods:

View File

@ -67,10 +67,6 @@ const Type *EncodePNode::Value( PhaseTransform *phase ) const {
}
Node *EncodeNarrowPtrNode::Ideal_DU_postCCP( PhaseCCP *ccp ) {
return MemNode::Ideal_common_DU_postCCP(ccp, this, in(1));
}
Node* DecodeNKlassNode::Identity(PhaseTransform* phase) {
const Type *t = phase->type( in(1) );
if( t == Type::TOP ) return in(1);

View File

@ -39,7 +39,6 @@ class EncodeNarrowPtrNode : public TypeNode {
}
public:
virtual uint ideal_reg() const { return Op_RegN; }
virtual Node *Ideal_DU_postCCP( PhaseCCP *ccp );
};
//------------------------------EncodeP--------------------------------

View File

@ -1387,12 +1387,6 @@ bool Node::remove_dead_region(PhaseGVN *phase, bool can_reshape) {
return false;
}
//------------------------------Ideal_DU_postCCP-------------------------------
// Idealize graph, using DU info. Must clone result into new-space
Node *Node::Ideal_DU_postCCP( PhaseCCP * ) {
return NULL; // Default to no change
}
//------------------------------hash-------------------------------------------
// Hash function over Nodes.
uint Node::hash() const {
@ -2081,6 +2075,14 @@ Node* Node::unique_ctrl_out() const {
return found;
}
void Node::ensure_control_or_add_prec(Node* c) {
if (in(0) == NULL) {
set_req(0, c);
} else if (in(0) != c) {
add_prec(c);
}
}
//=============================================================================
//------------------------------yank-------------------------------------------
// Find and remove

View File

@ -906,9 +906,6 @@ protected:
bool remove_dead_region(PhaseGVN *phase, bool can_reshape);
public:
// Idealize graph, using DU info. Done after constant propagation
virtual Node *Ideal_DU_postCCP( PhaseCCP *ccp );
// See if there is valid pipeline info
static const Pipeline *pipeline_class();
virtual const Pipeline *pipeline() const;
@ -942,6 +939,9 @@ public:
// Return the unique control out if only one. Null if none or more than one.
Node* unique_ctrl_out() const;
// Set control or add control as precedence edge
void ensure_control_or_add_prec(Node* c);
//----------------- Code Generation
// Ideal register class for Matching. Zero means unmatched instruction

View File

@ -1605,21 +1605,6 @@ void PhaseCCP::do_transform() {
C->set_root( transform(C->root())->as_Root() );
assert( C->top(), "missing TOP node" );
assert( C->root(), "missing root" );
// Eagerly remove castPP nodes here. CastPP nodes might not be
// removed in the subsequent IGVN phase if a node that changes
// in(1) of a castPP is processed prior to the castPP node.
for (uint i = 0; i < _worklist.size(); i++) {
Node* n = _worklist.at(i);
if (n->is_ConstraintCast()) {
Node* nn = n->Identity(this);
if (nn != n) {
replace_node(n, nn);
--i;
}
}
}
}
//------------------------------transform--------------------------------------
@ -1700,11 +1685,6 @@ Node *PhaseCCP::transform_once( Node *n ) {
_worklist.push(n); // n re-enters the hash table via the worklist
}
// Idealize graph using DU info. Must clone() into new-space.
// DU info is generally used to show profitability, progress or safety
// (but generally not needed for correctness).
Node *nn = n->Ideal_DU_postCCP(this);
// TEMPORARY fix to ensure that 2nd GVN pass eliminates NULL checks
switch( n->Opcode() ) {
case Op_FastLock: // Revisit FastLocks for lock coarsening
@ -1721,12 +1701,6 @@ Node *PhaseCCP::transform_once( Node *n ) {
default:
break;
}
if( nn ) {
_worklist.push(n);
// Put users of 'n' onto worklist for second igvn transform
add_users_to_worklist(n);
return nn;
}
return n;
}

View File

@ -0,0 +1,84 @@
/*
* Copyright (c) 2015, 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
* @bug 8069191
* @summary predicate moved out of loops and CastPP removal causes dependency to be lost
* @run main/othervm -Xcomp -XX:CompileOnly=TestPredicateLostDependency.m1 -XX:+IgnoreUnrecognizedVMOptions -XX:+StressGCM TestPredicateLostDependency
*
*/
public class TestPredicateLostDependency {
static class A {
int i;
}
static class B extends A {
}
static boolean crash = false;
static boolean m2() {
return crash;
}
static int m3(float[] arr) {
return 0;
}
static float m1(A aa) {
float res = 0;
float[] arr = new float[10];
for (int i = 0; i < 10; i++) {
if (m2()) {
arr = null;
}
m3(arr);
int j = arr.length;
int k = 0;
for (k = 9; k < j; k++) {
}
if (k == 10) {
if (aa instanceof B) {
}
}
res += arr[0];
res += arr[1];
}
return res;
}
static public void main(String args[]) {
A a = new A();
B b = new B();
for (int i = 0; i < 20000; i++) {
m1(a);
}
crash = true;
try {
m1(a);
} catch (NullPointerException npe) {}
}
}