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

[Roland Westrelin]

> Good.
> 
> I don't like *virtual* methods :) and we can do with (t->field_at(1 - _con) == Type::TOP) but your code is more clear.
> 
> May be change method to return bool:
> 
> virtual bool always_taken(const TypeTuple* t) const { return (t == TypeTuple::IFFALSE); }

Thanks for the review and comments. I’ll make that change before I push it.

Roland.

> 
> Thanks,
> Vladimir
> 
> 
> On 1/7/15 2:02 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 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.
>>>>>>>>>>> 
>>>>>>>>> 
>>>>>>> 
>>>>> 
>>