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 14:28:20 UTC 2017
>> 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