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

[Vladimir Kozlov]

So it is again _major_progress problem.
I have to spend more time on this. It is not simple.

We may add an other state when Ideal transformation could be executed. 
For example, after all loop opts:

http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/0fc557e05fc0/src/share/vm/opto/compile.cpp#l2286

Or more states to specify which Ideal transformations and loop 
optimizations could be executed in which state.

The main problem from your description is elimination of Opaque1 on 
which loop optimizations relies.

We can simply remove Opaque1Node::Identity(PhaseGVN* phase) because 
PhaseMacroExpand::expand_macro_nodes() will remove them after all loop 
opts. On other hand we may do want to execute some simple loop 
optimizations even after Opaque, CastII and CastI2L are optimized out. 
For example, removing empty loops or one iteration loops (pre-loops). 
But definitely not ones which use cloning or other aggressive optimizations.

Inline_Warm() is not used since InlineWarmCalls for very long time. The 
code could be very rotten by now. So removing set_major_progress from it 
is fine.

It is also fine to remove it from inline_incrementally since it will be 
restored by skip_loop_opts code (and cleared if method is empty or set 
if there are expensive nodes).

LoopNode::Ideal() change seems also fine. LoopNode is created only in 
loop opts (RootNode has own Ideal()) so if it has TOP input it will be 
removed by RegionNode::Ideal most likely.

Which leaves remove_useless_bool() code only and I have concern about 
it. It could happened after CCP phase and we may want to execute loop 
opts after it. I am actually want to set major progress after CCP 
unconditionally since some If nodes could be folded by it.

As you can see it is not simple :(

Thanks,
Vladimir

On 2/22/16 6:22 AM, Zoltán Majó wrote:
> 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
>>>
>