Request for reviews (S): 6791852: assert(b->_nodes[insidx] == n,"got insidx set incorrectly")

[Tom Rodriguez]

ifg.cpp:

I think I would have preferred if this were simply done as part of the  
backwards walk during ifg construction, so that there's no extra  
work.  This adds an extra pass over every block that almost never does  
anything.  I guess it's more clear this way.

The loop bounds are a little strange.  I think it should either be

+     for( uint insidx = last_phi + 1; insidx < last_inst; insidx++ ) {
+       Node *ex = b->_nodes[insidx];
+       if( ex->is_Mach() &&
+           ex->as_Mach()->ideal_Opcode() == Op_CreateEx ) {
+         b->_nodes.remove(insidx);
+         b->_nodes.insert(last_phi, ex);
+         break;
+       }
+     }

or

+     for( uint insidx = last_phi; insidx < last_inst; insidx++ ) {
+       Node *ex = b->_nodes[insidx];
+       if (ex->is_Mach() &&
+           ex->as_Mach()->ideal_Opcode() == Op_CreateEx ) {
+         if( insidx > last_phi) {
+           b->_nodes.remove(insidx);
+           b->_nodes.insert(last_phi, ex);
+         }
+         break;
+       }
+     }

Actually what about this:

      for( uint insidx = last_phi; insidx < last_inst; insidx++ ) {
        Node *ex = b->_nodes[insidx];
        if (ex->is_SpillCopy()) continue;
        if (insidx > last_phi && ex->is_Mach() &&
            ex->as_Mach()->ideal_Opcode() == Op_CreateEx) {
          // If the CreateEx isn't above all the MachSpillCopies
	 // then move it to the top
          b->_nodes.remove(insidx);
          b->_nodes.insert(last_phi, ex);
        }
        // Stop once a CreateEx or any other node is found
        break;
      }

This way we only scan as much as needed.

Can you correct the name of last_phi?  It's described as the index of  
that last_phi but it's actually the index of the first node after the  
phis.  Maybe first_inst instead?

Otherwise it looks good.

tom

On Feb 6, 2009, at 10:35 AM, Vladimir Kozlov wrote:

> http://cr.openjdk.java.net/~kvn/6791852/webrev.00
>
> Fixed 6791852: assert(b->_nodes[insidx] == n,"got insidx set  
> incorrectly")
>
> Problem:
> The code in PhaseChaitin::Split() in the part "Handle Crossing HRP  
> Boundry"
> expects that a new MachSpillCopy node is inserted before the current  
> node.
> After 6782232 fix it is not true if the current node is CreateEx.
>
> Solution:
> Remove the original fix for 6782232 in PhaseChaitin::insert_proj().
> Move the CreateEx up before each round of IFG construction to produce
> correct interference graph.
> Move verification checks in chaitin after IFG construction.
> Add additional check into PhaseCFG::verify().
> Fix check in PhaseChaitin::verify_base_ptrs().
>
> Reviewed by:
>
> Fix verified (y/n): y, bug's test case
>
> Other testing:
> JPRT, CTW (with -XX:+VerifyRegisterAllocator)
>