RFR: 8290781: Segfault at PhaseIdealLoop::clone_loop_handle_data_uses [v2]

[Tobias Hartmann]

On Fri, 26 Aug 2022 08:03:41 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> In the test case, both:
>> 
>>             v1 = array[i];
>> (in the first loop), and
>> 
>>             v += array[i-1];
>> 
>> (in the second one) access the same element. The bound check for the
>> second access is optimized out and the load of that access becomes
>> control dependent on the range check of the first one.
>> 
>> In the context of loop strip mining, data nodes that are in the outer
>> loop are expected to be reachable from the safepoint node. There are
>> rare case when it's not the case so I added logic to fix those cases
>> before loop cloning. That logic covers both nodes that have control in
>> the outer loop and control input in the outer loop. That logic is
>> incorrect (in the case of a node with a control input in the outer
>> loop): when cloning the first loop body, that logic finds the load
>> referenced from the range check in the loop body. That load has a
>> control input that's in the inner loop. But the logic only check
>> whether it's in the outer loop: anything in the inner loop is also in
>> the outer loop with that logic. The control of the load is then
>> wrongly updated to be outside the outer loop. That then causes a crash
>> because the load is recorded as being in the body of the second loop
>> but its control is not.
>> 
>> The fix I propose stengthen the logic that checks for nodes in the
>> outer loop: it checks that the control input is in the outer loop but
>> not in the inner loop.
>
> Roland Westrelin has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Update src/hotspot/share/opto/loopopts.cpp
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>  - Update src/hotspot/share/opto/loopopts.cpp
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>

Looks good to me. I'll run some testing and report back once it passed.

src/hotspot/share/opto/loopopts.cpp line 2168:

> 2166:       // nodes pinned with control in the outer loop but not referenced from the safepoint must be moved out of
> 2167:       // the outer loop too
> 2168:         Node* u_c = u->in(0);

Suggestion:

        // nodes pinned with control in the outer loop but not referenced from the safepoint must be moved out of
        // the outer loop too
        Node* u_c = u->in(0);

-------------

Marked as reviewed by thartmann (Reviewer).

PR: https://git.openjdk.org/jdk/pull/9997