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

[Vladimir Kozlov]

On 1/6/15 2:31 PM, Vladimir Kozlov wrote:
> 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.

Add IfNode to has_special_unique_user() so the code in 
PhaseIterGVN::remove_globally_dead_node() will do it for you.

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