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

[Vladimir Kozlov]

On 1/14/19 3:49 PM, Vladimir Ivanov wrote:
> 
>>>
>>> But it looks more like a workaround and I decided to go with the fix in PhaseIdealLoop::spinup() because I don't see 
>>> a reason why IDOM recomputation can't be triggered from other places.
>>
>> I am not sure your changes help to all cases.  It may indeed helps to split_if optimization but dominator information 
>> is used before it too. I see Shenandoah's optimize_loops() uses information before split_if.
> 
> I try to address only PhaseIdealLoop::spinup() case. There may be other bugs lurking in other places.
> 
>> Can we correctly recalculate IDOM after counted_loop() if strip mining loop was inserted? My be we can simplify strip 
>> mining code if we know that IDOM will be recalculated.
> 
> The simplest fix I can come up with (and most reliable IMO w.r.t. other possible bugs which aren't uncovered yet) is to 
> set C->major_progress() if strip mining happened and return early to initiate the next round of PhaseIdealLoop and 
> recompute IDOM info. In that case, transformations will see only IDOM computed by Dominators(), but it means repeated 
> IDOM & loop info computations when strip mining happens.

Yes. It is safest/conservative solution. The only issue is that we have several targeted individual calls to 
PhaseIdealLoop before we go into optimize_loops() which calls PhaseIdealLoop in loop. So initial PhaseIdealLoop call 
sequence could be altered if we bailout too soon.

> 
>> Would be nice to hear Roland's opinion too.
> 
> Yes, same here.
> 
> As for me:
> 
>   * I find it ugly that Dominators() and dom_lca() aren't consistent;

Agree. Should be fixed (but not urgent for 12).

> 
>   * I'm in favor of normalized info (dom_lca() variant) to be computed from the very beginning;

File RFE.

> 
>   * I still believe PhaseIdealLoop::spinup() has a bug which should be fixed (irrespective of whether IDOM is normalized 
> or not);

I agree with that too.

Again, I agree with your fix for jdk 12. Lets clean up this mess after that.

Thanks,
Vladimir

> 
> Best regards,
> Vladimir Ivanov