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

[Vladimir Kozlov]

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