RFR [9] (XS): 8179882: C2: Stale control info after cast node elimination during loop optimization pass
Vladimir Kozlov
vladimir.kozlov at oracle.com
Sat May 27 00:22:56 UTC 2017
Looks good.
Thanks,
Vladimir
On 5/26/17 6:09 AM, Vladimir Ivanov wrote:
> How about the following version?
> http://cr.openjdk.java.net/~vlivanov/8179882/webrev.01/
>
> Best regards,
> Vladimir Ivanov
>
> On 5/25/17 8:23 PM, Vladimir Kozlov wrote:
>> 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