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

[Tobias Hartmann]

Thanks, Vladimir!

Best,
Tobias

On 18.12.2015 20:00, Vladimir Kozlov wrote:
> 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
>>