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

[Vladimir Kozlov]

Thanks, Tom

I will use the last one with is_SpillCopy() check.
and I will rename the variable.

Thanks,
Vladimir

Tom Rodriguez wrote:
> 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)
>>
>