[9] RFR(S): 8145754: PhaseIdealLoop::is_scaled_iv_plus_offset() does not match AddI

[Vladimir Kozlov]

Looks good.

Thanks,
Vladimir

On 12/18/15 6:52 AM, Tobias Hartmann wrote:
> Hi,
>
> please review the following patch.
>
> https://bugs.openjdk.java.net/browse/JDK-8145754
> http://cr.openjdk.java.net/~thartmann/8145754/webrev.00/
>
> PhaseIdealLoop::is_scaled_iv_plus_offset() returns false if the expression is an AddI node with the scaled iv as second input because is_scaled_iv() is only invoked for exp->in(1). We need to check exp->in(2) as well like we do for the SubI node (and also in SWPointer::scaled_iv_plus_offset()).
>
> Background:
> I caught this bug with my prototype fix for JDK-6675699 while investigating a regression with the SPECjvm2008 mpeg benchmark. It turned out that with my fix I was hitting the LoopUnrollLimit for loops in some hot methods and therefore the loops were not completely unrolled/removed. Compared to the baseline version, some RC predicates were not emitted and therefore range checks in the loop body were not eliminated. As a result, the loop body node count was too high for unrolling. The RC predicates were not added because IdealLoopTree::is_range_check_if() uses is_scaled_iv_plus_offset() which fails. I think the input nodes of the AddI node are swapped with my fix because the node indices are slightly different and commute() (see addnode.cpp) sorts inputs according to their index:
>    // Otherwise, sort inputs (commutativity) to help value numbering.
>    if( in1->_idx > in2->_idx ) {
>      add->swap_edges(1, 2);
>      return true;
>    }
>
> I verified that this also happens without my fix for JDK-6675699 by adding
>    assert(!is_scaled_iv(exp->in(2), iv, p_scale), "Found input at position 2!");
> and running JPRT. As expected, we hit the assert.
>
> Thanks,
> Tobias
>