RFR(S): 8231291: C2: loop opts before EA should maximally unroll loops

[Roland Westrelin]

Hi Vladimir,

Thanks for reviewing this.

> As I understand, the intention of the change you propose is to perform 
> complete loop unrolling earlier so EA can benefit from it.

Yes.

> It looks like LoopOptsMaxUnroll is a shortened version of 
> IdealLoopTree::iteration_split/iteration_split_impl().

Yes.

> Have you considered factoring out the common code? Right now, its hard 
> to correlate the checks for LoopOptsMaxUnroll with iteration_split() and 
> there's a risk they'll diverge eventually.

No I haven't. But it seems it's either we duplicate the code as I did or
we add some flag to iteration_split() and make some of the code their
conditional. Wouldn't that affect clarity and for what's overall a
fairly simple change?

> Do you need the following steps from the original version?
>
> ================================================
>    // Look for loop-exit tests with my 50/50 guesses from the Parsing stage.
>    // Replace with a 1-in-10 exit guess.
>    if (!is_root() && is_loop()) {
>      adjust_loop_exit_prob(phase);
>    }
>
>    // Compute loop trip count from profile data
>    compute_profile_trip_cnt(phase);
>
> No use of profiling data since full unrolling is happening anyway?

Right.

> ========================
>    if (!cl->is_valid_counted_loop()) return true; // Ignore various 
> kinds of broken loops

policy_maximally_unroll() has that check.

> ========================
>    // Do nothing special to pre- and post- loops
>    if (cl->is_pre_loop() || cl->is_post_loop()) return true;
>
> I assume there are no pre-/post-loops exist at that point, so these 
> checks are redundant. Turn them into asserts?

I added a check for is_normal_loop() to be safe.

> ========================
>    if (cl->is_normal_loop()) {
>      if (policy_unswitching(phase)) {
>        phase->do_unswitching(this, old_new);
>        return true;
>      }
>      if (policy_maximally_unroll(phase)) {
>        // Here we did some unrolling and peeling.  Eventually we will
>        // completely unroll this loop and it will no longer be a loop.
>        phase->do_maximally_unroll(this, old_new);
>        return true;
>      }
>
> You don't perform loop unswitching at all. So, the order of operations 
> changes. Do you see any problems with that?

I'm not sure what to think about that one. Yes, I suppose it could be
that unswitching helps but if we unswitch we have to perform 2 passes of
loop opts to get to the maximally unrolled loop. Isn't that more
disruptive that we would want?

> ========================
>
>
> src/hotspot/share/opto/compile.cpp
>     // Perform escape analysis
>     if (_do_escape_analysis && ConnectionGraph::has_candidates(this)) {
>       if (has_loops()) {
>         // Cleanup graph (remove dead nodes).
>         TracePhase tp("idealLoop", &timers[_t_idealLoop]);
> -      PhaseIdealLoop::optimize(igvn, LoopOptsNone);
> +      PhaseIdealLoop::optimize(igvn, LoopOptsMaxUnroll);
>         if (major_progress()) print_method(PHASE_PHASEIDEAL_BEFORE_EA, 2);
>         if (failing())  return;
>       }
>       ConnectionGraph::do_analysis(this, &igvn);
>
> Does it make sense to do more elaborate checks before performing early 
> full loop unrolling? Like whether candidates are used inside loop bodies?

We would maximally unroll the loop anyway, only a bit later so I don't
think we need to make this more involved that it needs to be.

new webrev:

http://cr.openjdk.java.net/~roland/8231291/webrev.02/

Roland.