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

[Vladimir Kozlov]

On 1/6/15 1:44 PM, Roland Westrelin wrote:
>> 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.

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