SUGGESTED FIX
Cliff's suggested fix is too restrictive. It fails to find invariant some cases in which are currently correctly found so. First, instead of looking for a "unique" node,
we should just push adr->in(LoopNode::EntryControl) onto closure. The remaining logic regarding "unique" goes away. Second, because we are now pushing all of the CastPP nodes onto the closure list, we need to expand the sanity check, probably 2x.
|
SUGGESTED FIX
Cliff Click suggests:
//---------------------------cast_is_loop_invariant----------------------------
// A helper function for Ideal_DU_postCCP to check if a CastPP in a
counted
-// loop is loop invariant. Make a quick traversal of Phi and CastPP nodes
+// loop is loop invariant. Make a quick traversal of Phi and CastPP nodes
// looking to see if they are a closed group within the loop.
static bool cast_is_loop_invariant(Node* cast, Node* adr) {
- // The idea is that the phi-nest must boil down to only CastPP nodes
- // with the same data input as the original cast. 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.
+ // 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;
Unique_Node_List closure;
- closure.push(cast);
- closure.push(adr);
-
Unique_Node_List worklist;
- worklist.push(adr->in(LoopNode::LoopBackControl));
+ worklist.push(adr);
+ if( cast ) worklist.push(cast);
- int op = cast->Opcode();
- Node* input = cast->in(1);
+ Node *unique = NULL; // Do not know the original data
// Begin recursive walk of phi nodes.
while( worklist.size() ){
@@ -279,19 +275,18 @@
// Make a sanity check to ensure we don't waste too much time here.
if( closure.size() > 10) return false;
// This node is OK if:
- // - it is a cast identical (based on opcode and input) to the
original one
+ // - 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->Opcode() == op ) {
- if( n->in(1) != input ){
- return false;
- }
+ if( n->Opcode() == Op_CastPP ) {
+ worklist.push(n->in(1)); // Check input
} else if( n->Opcode() == Op_Phi ) {
- for( uint i = 1; i < n->req(); i++ ) {
- worklist.push(n->in(i));
- }
- } else {
- return false;
+ for( uint i = 1; i < n->req(); i++ )
+ worklist.push(n->in(i)); // Check all inputs
+ } else if( !unique ) { // Unique input not discovered yet?
+ unique = n; // Record unique input
+ } else if( unique != n ) {// Must be equal to unique
+ return false; // Merge of unequals
}
}
}
@@ -317,7 +312,7 @@
Node *ctr = in(MemNode::Control);
Node *mem = in(MemNode::Memory);
Node *adr = in(MemNode::Address);
- Node *elided_cast = NULL;
+ 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.
@@ -326,82 +321,84 @@
case Op_Phi:
// Attempt to float above a Phi to some dominating point.
if( 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( cast_is_loop_invariant(skipped_cast, adr)) {
+ adr = adr->in(1);
+ continue;
+ }
+ }
- // If we've already peeked through a CastPP (which could have set the
- // control), we can't float above a Phi, because the ignored CastPP
- // may not be loop invariant.
- if( elided_cast == NULL ||
- cast_is_loop_invariant(elided_cast, adr)) {
- adr = adr->in(1);
- continue;
- }
- }
+ // Intentional fallthrough!
|
Java