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

[Vladimir Ivanov]

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