RFR [9] (XS): 8179882: C2: Stale control info after cast node elimination during loop optimization pass
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Thu May 25 00:50:12 UTC 2017
On 5/25/17 3:24 AM, Vladimir Kozlov wrote:
> 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.
Sorry for the confusion. #1 & #2-#3 happen in different (consequent)
loop opts iterations. Loop info is rebuilt after #2 and it goes out of
sync during #2-#3.
> 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?
Cast node elimination in split_if_with_blocks_pre doesn't bump
_major_progress, so after split-ifs are over (#2-#3 are part of them),
the compiler proceeds to loop opts.
Calling set_major_progress() when dom_cast doesn't dominate n in
split_if_with_blocks_pre() is another way to fix the bug, but it is much
more expensive (introduces additional loop opts pass).
Best regards,
Vladimir Ivanov
>>
>> #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.
More information about the hotspot-compiler-dev
mailing list