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

[Vladimir Ivanov]

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