[12] RFR (S): 8215757: C2: PhaseIdealLoop::spinup() computes wrong post-dominating point

[Vladimir Kozlov]

Looks good.

I tried to look on history of this code and it was from the first day of loop predicates implementation:

http://hg.openjdk.java.net/jdk9/hs/hotspot/rev/b2b6a9bf6238#l5.184

Thanks,
Vladimir

On 1/15/19 5:21 PM, Vladimir Ivanov wrote:
> Updated webrev (with Roland's proposal):
>    http://cr.openjdk.java.net/~vlivanov/8215757/webrev.01/
> 
> Testing: failing test (replay), hs-precheckin-comp, hs-tier1, hs-tier2 (in progress)
> 
> Best regards,
> Vladimir Ivanov
> 
> On 15/01/2019 09:46, Vladimir Ivanov wrote:
>>
>>
>> On 15/01/2019 05:38, Roland Westrelin wrote:
>>>
>>> Hi Vladimir K & Vladimir I,
>>>
>>>>> And that's the other way to fix the crash: initiate new PhaseIdealLoop iteration right away if 
>>>>> any strip mined loops are
>>>>> introduced.
>>>>
>>>> Got it. So the issue is that strip mining invalidated IDOM information generated at the 
>>>> beginning of
>>>> PhaseIdealLoop::build_and_optimize().
>>>
>>> I don't think that's accurate. The idom is changed when a loop limit
>>> check is inserted (so that's unrelated to strip mining AFAICT). As
>>> Vladimir said, when the loop limit check is inserted, the idom of the
>>> region is fixed by:
>>>
>>>      Node* nrdom = dom_lca(ridom, new_iff);
>>>      set_idom(rgn, nrdom, dom_depth(rgn));
>>>
>>> which does:
>>>
>>>    Node *dom_lca( Node *n1, Node *n2 ) const {
>>>      return find_non_split_ctrl(dom_lca_internal(n1, n2));
>>>    }
>>>
>>> and because of the find_non_split_ctrl(), the idom is set to a region
>>> rather than an if.
>>>
>>> That's broken and I'm confused as to why a straightforward change of the
>>> logic above:
>>>
>>> diff --git a/src/hotspot/share/opto/loopPredicate.cpp b/src/hotspot/share/opto/loopPredicate.cpp
>>> --- a/src/hotspot/share/opto/loopPredicate.cpp
>>> +++ b/src/hotspot/share/opto/loopPredicate.cpp
>>> @@ -160,7 +160,7 @@
>>>     // When called from beautify_loops() idom is not constructed yet.
>>>     if (_idom != NULL) {
>>>       Node* ridom = idom(rgn);
>>> -    Node* nrdom = dom_lca(ridom, new_iff);
>>> +    Node* nrdom = dom_lca_internal(ridom, new_iff);
>>>       set_idom(rgn, nrdom, dom_depth(rgn));
>>>     }
>>> is not good enough.
>>
>> Fair point. So you're saying that dom_lca()/find_non_split_ctrl() should never be used to set 
>> IDOM, right? And all the places which require non-split point for an IDOM should explicitly 
>> normalize it?
>>
>> I checked the codebase and all places, where dom_lca()/find_non_split_ctrl() are used, IDOM is 
>> left intact except (PhaseIdealLoop::create_new_if_for_predicate).
>>
>> So, I'm fine with the fix you propose (though I'm still not happy about the distinction between 
>> IDOM & dom_lca()/find_non_split_ctrl()).
>>
>> Best regards,
>> Vladimir Ivanov