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

[Vladimir Kozlov]

Changpeng Fang wrote:
> 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?

Yes.

>> 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.

OK.

Vladimir

> 
> 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
>