RFR [9] (XS): 8179882: C2: Stale control info after cast node elimination during loop optimization pass

[Vladimir Kozlov]

On 5/24/17 4:38 PM, Vladimir Ivanov wrote:
>>>>> The problem with it is that ConstraintCastNode::dominating_cast()
>>>>> relies on immediate control info (in(0)) which can get out of sync
>>>>> with what is cached in PhaseIdealLoop (get_ctrl()).
>>>>
>>>> Should we fix this instead? It means somewhere we forgot to update loop
>>>> info.
>>>
>>> Can you elaborate your point, please? The fix being proposed does
>>> exactly that - detect the case when dom_cast should be moved and
>>> updates its control & loop info to dominate all users
>>> (dom_lca(dom_cast_ctrl, n_ctrl)).
>>
>> Could be misunderstanding. You said get_ctrl() does not point to in(0)
>> at time when dominating_cast() is called. My question is why? During
>> loop construction it should point to it. At which time it is changed?
> 
> Step-by-step description of applied transformations follows (details [1]):
>    #1: If: 1012 => 1257
>       - same condition (1820 Bool), 1257 dominates 1012

Which transformation replace 1012 with 1257? We should exit loop opts 
after such transformation is done and recalculate new loop info again on 
next loop opts iteration.

Based on code it look like we are doing split_if_with_blocks when this 
happens. major_progress should be set and exit this iteration of loop 
opts. So why "loop cloning" happens?

Vladimir

> 
>    #2: CastPP: 5846 => 733
>       - 733 dominates 5846
> 
>    #3: CheckCastPP: 773 => 574
>       - 773 & 574 have the same control node after #1 (1260/1257), but 
> get_ctrl(dom_cast) points to 1581 (before #2: 574 -1-> 5846 -0-> 1581).
> 
> The problem is that 773 was part of N4442 and 574 doesn't dominate the 
> loop body, but #3 doesn't place 574 there when replacing 773. So, 
> subsequent cloning of loop body during loop peeling of N4442 produces 
> unschedulable graph.
> 
> The fix moves 574 up the dominator tree and puts it inside N4442, so it 
> dominates all users of 773 & 574 after #3.
> 
> get_ctrl() & in(0) point to different CFG nodes after #1, but the loop 
> info is correct until #3 happens which requires 574 to be moved up.
> 
> Best regards,
> Vladimir Ivanov
> 
> [1]
> === new loop opt pass
> 
> #0: Initial state
>    Loop: N4442/N1838  body={ 1257 773 1260 }
>    Loop: N4445/N2197  body={ 1012 574 1075 }
> 
>   574    CheckCastPP    ===  1075  5846 
> Oop:java/lang/Integer:NotNull:exact *
>       1075    IfFalse    ===  1012
>           1012    If    ===  1575  1820
>               1820    Bool    ===  ...
>       5846    CastPP    ===  1581  677
>           1581    Proj    ===  2137 #0  Type:control
>               2137    Unlock    === ...
>           677    Phi    === ...
> 
>   773    CheckCastPP    ===  1260  733 
> Oop:java/lang/Integer:NotNull:exact *
>       1260    IfFalse    ===  1257
>           1257    If    ===  1819  1820
>               1820    Bool    === ...
>       733    CastPP    ===  1230  677
>           1230    IfTrue    ===  1225
>               1225    If    === ...
>           677    Phi    === ...
> 
> 
> #1: 1012 => 1257 (same condition, 1257 dominates 1012) and 574 isn't 
> part of N4445 anymore
> 
>    Loop: N4442/N1838  body={ 1257 773 1260 }
>    Loop: N4445/N2197  body={ }
> 
>   574    CheckCastPP    ===  1260  5846
>       1260    IfFalse    ===  1257
>           1257    If    ===  1575  1820
>               1820    Bool    ===  ...
>       5846    CastPP    ===  1581  677
>           1581    Proj    ===  2137 #0  Type:control
>               2137    Unlock    === ...
>           677    Phi    === ...
> 
>   773    CheckCastPP    ===  1260  733
>       1260    IfFalse    ===  1257
>           1257    If    ===  1819  1820
>               1820    Bool    === ...
>       733    CastPP    ===  1230  677
>           1230    IfTrue    ===  1225
>               1225    If    === ...
>           677    Phi    === ...
> 
> === next loop opts pass
> 
>    Loop: N4442/N1838  body={ 1257 773 1260 }
>    Loop: N4445/N2197  body={ }
> 
> #2: 5846 => 733
> 
>   574    CheckCastPP    ===  1260  733
>   773    CheckCastPP    ===  1260  733
> 
>       1260    IfFalse    ===  1257
>           1257    If    ===  1819  1820
>               1820    Bool    === ...
> 
>       733    CastPP    ===  1230  677
>           1230    IfTrue    ===  1225
>               1225    If    === ...
>           677    Phi    === ...
> 
>    Loop: N4442/N1838  body={ 1257 773 1260 }
>    Loop: N4445/N2197  body={ }
> 
> #3: 773 => 574
> 
>    Loop: N4442/N1838  body={ 1257 1260 } // NB! missing 574
>    Loop: N4445/N2197  body={ }
> 
> #4: Peel N4442 (NB! same loop opt iteration, so loop tree hasn't been 
> rebuilt yet.)
>    - 574 isn't cloned since it's not part of the loop body and it leads 
> to unscheduleable IR.
> 
> === Crash
> 
>> If there was a new node created during loopopts then
>> PhaseIdealLoop::register_new_node() should be called for it already.
>>
>> Vladimir K
>>
>>>
>>> Or do you suggest to rewrite ConstraintCastNode::dominating_cast() to
>>> use get_ctrl() instead of in(0)?
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>>>>
>>>>> The fix is to catch the case when dom_cast doesn't dominate n based on
>>>>> info from PhaseIdealLoop and update control info accordingly.
>>>>>
>>>>> Testing: manual (replayed problematic compilation & eyeballed the IR),
>>>>> JPRT, RBT (hs-tier0-comp, in progress).
>>>>>
>>>>> Best regards,
>>>>> Vladimir Ivanov
>>>>>
>>>>> PS: thanks to Roland for helping with the fix.