RFR [9] (XS): 8179882: C2: Stale control info after cast node elimination during loop optimization pass
Vladimir Kozlov
vladimir.kozlov at oracle.com
Thu May 25 17:23:12 UTC 2017
Okay, now I understand the problem.
An other thing I do not like about original change is that it could
place dom_cast above its inputs which could be incorrect:
set_ctrl_and_loop(dom_cast, dom_lca(dom_cast_ctrl, n_ctrl));
I like [1] approach which do replacement only with clear domination.
Also would be nice if you add comment in this code explaining it.
Thanks,
Vladimir
On 5/25/17 7:28 AM, Vladimir Ivanov wrote:
>>> 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.
>>
>> You mean: rebuild after #1. Right?
>
> Yes, typo.
>
>>>> 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.
>>
>> Rereading this part:
>>
>>>>> 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,
>>
>> Would it help if 574 CheckCastPP is replaced with 773?
>
> Yes, that's another way to fix it [1]. Either one or the other cast node
> dominates. If we are unlucky, compiler encounters the "wrong one"
> (dominator) first and tries to replace it with a dominated one. If the
> compiler skips it, it will eventually find dominated node and will
> attempt to replace it with the dominator (which will succeed).
>
> BTW it's equivalent to using get_ctrl() instead of in(0) in
> ConstraintCastNode::dominating_cast() when called with PhaseIdealLoop.
>
> Let me know what approach to fix the bug you prefer.
>
> Best regards,
> Vladimir Ivanov
>
> [1]
> diff --git a/src/share/vm/opto/loopopts.cpp
> b/src/share/vm/opto/loopopts.cpp
> --- a/src/share/vm/opto/loopopts.cpp
> +++ b/src/share/vm/opto/loopopts.cpp
> @@ -913,7 +913,7 @@
>
> if (n->is_ConstraintCast()) {
> Node* dom_cast = n->as_ConstraintCast()->dominating_cast(this);
> - if (dom_cast != NULL) {
> + if (dom_cast != NULL && is_dominator(get_ctrl(dom_cast),
> get_ctrl(n)) {
> _igvn.replace_node(n, dom_cast);
> return dom_cast;
> }
>
>>> 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