[9] RFR (S): 8148754: C2 loop unrolling fails due to unexpected graph shape

[Zoltán Majó]

Hi Vladimir,


thank you for the feedback!

On 02/16/2016 01:11 AM, Vladimir Kozlov wrote:
> Zoltan,
>
> It should not be "main" loop if peeling happened. See do_peeling():
>
>     if (cl->is_main_loop()) {
>       cl->set_normal_loop();
>
> Split-if optimization should not split through loop's phi. And 
> generally not through loop's head since it is not making code better - 
> split through backedge moves code into loop again. Making loop body 
> more complicated as this case shows.

I did more investigation to understand what causes the invalid graph 
shape to appear. It seems that the invalid graph shape appears because 
the compiler uses the Compile:: _major_progress inconsistently. Here are 
some details.

- If _major_progress *is set*, the compiler expects more loop 
optimizations to happen. Therefore, certain transformations on the graph 
are not allowed so that the graph is in a shape that can be processed by 
loop optimizations. See:
http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/2c3c43037e14/src/share/vm/opto/convertnode.cpp#l253
http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/2c3c43037e14/src/share/vm/opto/castnode.cpp#l251
http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/2c3c43037e14/src/share/vm/opto/loopnode.cpp#l950
http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/2c3c43037e14/src/share/vm/opto/opaquenode.cpp#l37

- If _major_progress *is not set*, the compiler is allowed to perform 
all possible transformations (because it does not have to care about 
future loop optimizations).

The crash reported for the current issue appears because _major_progress 
*can be accidentally set again* after the compiler decided to stop 
performing loop optimizations. As a result, invalid graph shapes appear.

Here are details about how this happens for both failures I've been 
studying:
https://bugs.openjdk.java.net/browse/JDK-8148754?focusedCommentId=13901941&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13901941

I would propose to change the compiler to use _major_progress 
consistently. (This goes into the same direction as Tobias's recent work 
on JDK-8144487.)

I propose that _major_progress:
- can be SET when the compiler is initialized (because loop 
optimizations are expected to happen afterwards);
- can be SET/RESET in the scope of loop optimizations (because we want 
to see if loop optimizations made progress);
- cannot be SET/RESET by neither incremental inlining nor IGVN (even if 
the IGVN is performed in the scope of loop optimizations).

Here is the updated webrev:
http://cr.openjdk.java.net/~zmajo/8148754/webrev.02/

Performance evaluation:
- The proposed webrev does not cause performance regressions for 
SPECjvm2008, SPECjbb2005, and Octane.

Testing:
- all hotspot JTREG tests on all supported platforms;
- JPRT;
- failing test case.

Thank you and best regards,


Zoltan


>
> Bailout unrolling is fine but performance may suffer because in some 
> cases loop unrolling is better then split-if.


>
> Thanks,
> Vladimir
>
> On 2/15/16 7:22 AM, Zoltán Majó wrote:
>> Hi,
>>
>>
>> please review the patch for 8148754.
>>
>> https://bugs.openjdk.java.net/browse/JDK-8148754
>>
>> Problem: Compilation fails when the C2 compiler attempts loop unrolling.
>> The cause of the failure is that the loop unrolling optimization expects
>> a well-defined graph shape at the entry control of a 'CountedLoopNode'
>> ('IfTrue'/'IfFalse' preceeded by 'If' preceeded by 'Bool' preceeded by
>> 'CmpI').
>>
>>
>> Solution: I investigated several different instances of the same
>> failure. It turns out that the shape of the graph at a loop's entry
>> control is often different from the way loop unrolling expects it to be
>> (please find some examples in the bug's JBS issue). The various graph
>> shapes are a result of previously performed transformations, e.g.,
>> split-if optimization and loop peeling.
>>
>> Loop unrolling requires the above mentioned graph shape so that it can
>> adjust the zero-trip guard of the loop. With the unexpected graph
>> shapes, it is not possible to perform loop unrolling. However, the graph
>> is still in a valid state (except for loop unrolling) and can be used to
>> produce correct code.
>>
>> I propose that (1) we check if an unexpected graph shape is encountered
>> and (2) bail out of loop unrolling if it is (but not fail in the
>> compiler in such cases).
>>
>> The failure was triggered by Aleksey's Indify String Concatenation
>> changes but the generated bytecodes are valid. So this seems to be a
>> compiler issue that was previously there but was not yet triggered.
>>
>>
>> Webrev:
>> http://cr.openjdk.java.net/~zmajo/8148754/webrev.00/
>>
>> Testing:
>> - JPRT;
>> - local testing (linux-86_64) with the failing test case;
>> - executed all hotspot tests locally, all tests pass that pass with an
>> unmodified build.
>>
>> Thank you!
>>
>> Best regards,
>>
>>
>> Zoltan
>>