RFR [9] (XS): 8179882: C2: Stale control info after cast node elimination during loop optimization pass
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Fri May 26 13:09:25 UTC 2017
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