RFR (S): 8215757: C2: PhaseIdealLoop::spinup() computes wrong post-dominating point
vladimir.kozlov at oracle.com
Tue Jan 15 00:08:37 UTC 2019
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;
> * 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.
> Best regards,
> Vladimir Ivanov
More information about the hotspot-compiler-dev