RFR(XS): 8027626: assert(Opcode() != Op_If || outcnt() == 2) failed: bad if #1

[Roland Westrelin]

>>>> Can we simple use replace_input_of(in(0), 0, NULL) in
>>>> If{False,True}Node::Identity()? It will disconnect IfNode's control
>>>> edge and put it on worklist.
>>> 
>>> But that’s a PhaseIterGVN’s method and Identity is passed a
>>> PhaseTransform?
>> 
>> An other suggestion is to delay If{False,True}Node::Identity()
>> optimization until not taken branch is removed: (outcnt() < 2). But it
>> will need to place taken ProjNode back on worklist when we removed other
>> branch.
> 
> Add IfNode to has_special_unique_user() so the code in PhaseIterGVN::remove_globally_dead_node() will do it for you.


Thanks for the suggestion, Vladimir. Does that look ok?

http://cr.openjdk.java.net/~roland/8027626/webrev.01/

Roland.

> 
> Thanks,
> Vladimir
> 
>> 
>>> 
>>> It seems in other locations in the code, the code is robust to broken
>>> if subgraphs. This for instance:
>> 
>> I think it is too late as this bug shows. We should not allow 2 control
>> users at any time in a graph (except for IfNode)!
>> 
>> Vladimir
>> 
>>> 
>>> IfNode::Ideal()
>>> 
>>>   // Another variation of a dead if
>>>   if (outcnt() < 2)  return NULL;
>>> 
>>> Why isn’t it good enough here?
>>> 
>>> Roland.
>>> 
>>>> 
>>>> Thanks,
>>>> Vladimir
>>>> 
>>>> On 1/6/15 5:38 AM, Roland Westrelin wrote:
>>>>>> Is it again the problem with IGVN list processing order?
>>>>>> What happens in a simple test case when only one branch is taking?
>>>>>> How projection and If nodes are removed?
>>>>> 
>>>>> IfNode::Value() returns either TypeTuple::IFFALSE or TypeTuple::IFTRUE
>>>>> If{False,True}Node::Identity() replaces the taken projection with
>>>>> the If’s control input.
>>>>> ProjNode::Value() returns top for the non taken projection and the
>>>>> dead branch is killed by propagating top.
>>>>> 
>>>>> So yes, it’s an IGVN list processing order problem.
>>>>> 
>>>>> Roland.
>>>>> 
>>>>> 
>>>>>> 
>>>>>> Vladimir
>>>>>> 
>>>>>> On 1/5/15 1:33 PM, Roland Westrelin wrote:
>>>>>>> Hi Vladimir,
>>>>>>> 
>>>>>>> Thanks for looking at this.
>>>>>>> 
>>>>>>>> I think correct fix will be eager dead 12212 If node elimination
>>>>>>>> when 12214 is replaced by 18733. Keep it connected to graph can
>>>>>>>> cause problems in other places.
>>>>>>> 
>>>>>>> IfFalseNode::Identity() is what optimizes 12212/12214 out. Should
>>>>>>> I make it an Ideal transformation so that I can call
>>>>>>> igvn->remove_dead_node() on the dead branch?
>>>>>>> 
>>>>>>> Roland.
>>>>>>> 
>>>>>>>> 
>>>>>>>> Thanks,
>>>>>>>> Vladimir
>>>>>>>> 
>>>>>>>> On 1/5/15 7:48 AM, Roland Westrelin wrote:
>>>>>>>>> http://cr.openjdk.java.net/~roland/8027626/webrev.00/
>>>>>>>>> 
>>>>>>>>> The following subgraph is where the bug shows up:
>>>>>>>>> 
>>>>>>>>> If (18732)
>>>>>>>>> |     \
>>>>>>>>> IfTrue IfFalse
>>>>>>>>> (18734) (18733)
>>>>>>>>> | |
>>>>>>>>> | If (12212)
>>>>>>>>> | |     \
>>>>>>>>> | IfFalse IfTrue
>>>>>>>>> | (12214)
>>>>>>>>> | |
>>>>>>>>> Region (12183)
>>>>>>>>> 
>>>>>>>>> Condition to 12212 is always false so 12214 is replaced by 18733
>>>>>>>>> and both branches of If 18732 are directly connected to Region
>>>>>>>>> 12183. 18733 still has dead 12212 as output.
>>>>>>>>> 12183 doesn't have phis so when it's transformed, If 18732 is
>>>>>>>>> considered for removal. IfTrue 18734 doesn't have uses anymore
>>>>>>>>> so it goes away but IfFalse 18733 still has some (dead branch
>>>>>>>>> 12212 is not yet removed).
>>>>>>>>> An If in the dead branch 12212 is processed. Range check
>>>>>>>>> smearing follows dominator controls until 18733, tests whether
>>>>>>>>> the If is a range check and the assert fires because the If only
>>>>>>>>> has one projection.
>>>>>>>>> 
>>>>>>>>> So we’re trying to optimize a dead branch. I fixed it by making
>>>>>>>>> the range check code more robust.
>>>>>>>>> 
>>>>>>>>> Roland.
>>>>>>>>> 
>>>>>>> 
>>>>> 
>>>