Request for review (XS): 6901572 JVM 1.6.16 crash on loops: assert(has_node(i),"")

[Changpeng Fang]

On 12/02/09 11:40, Vladimir Kozlov wrote:
> Changpeng,
>
> Update the comment in loopnode.cpp for dead phi check.
>
//Look for other phis (secondary IVs). Skip dead ones.
Is this ok?
> So we hit the assert in build_loop_late because in
> the secondary IV code we added new users of the phi
> and set control for them (phase->set_ctrl(add, cl)).
> Is this assumption correct? Or it was different reason?
>
No, we hit the assert in counted_loop when we set_early_ctrl:

  Node* diff = new (C, 3) SubINode(init2, ratio_init);
  phase->_igvn.register_new_node_with_optimizer(diff, init2);
  phase->set_early_ctrl(diff); // <--- will lead to assert

 The direct reason is that init2 is dead, where:
 Node *init2 = phi2->in( LoopNode::EntryControl );

 But if phi2 (secondary IV) is not dead,  init2 should not be dead.
 So, I simply filter out a broader case where the secondary IV is
 dead.

Thanks,

Changpeng 






> Vladimir
>
> Changpeng Fang wrote:
>> http://cr.openjdk.java.net/~cfang/6901572/webrev.01/
>>
>> Fixes 6901572 JVM 1.6.16 crash on loops: assert(has_node(i),"")
>>
>> Problem:
>> When the loop optimizer handles secondary induction variables in 
>> IdealLoopTree::counted_loop,
>> it hits a dead phi and results in "assert(has_node(i),"")" at the 
>> time of  "get_ctrl" for the dead node.
>> The phi is dead because "final int x" in the attached test case is 
>> useless and becomes dead after the
>> safepoint is removed for the counted loop.
>>
>> Solution:
>> Skip the secondary induction variable handling if it is dead. The 
>> dead phi will be put into the deadlist
>> late in PhaseIdealLoop::build_loop_late and removed.
>>
>> Test:
>> test case in the bug report.
>>
>> Thanks,
>>
>> Changpeng