RFR: 8315920: C2: "control input must dominate current control" assert failure
Tobias Hartmann
thartmann at openjdk.org
Fri Sep 15 13:50:47 UTC 2023
On Wed, 13 Sep 2023 14:35:55 GMT, Roland Westrelin <roland at openjdk.org> wrote:
> The assert fires during range check eliminationin code that was added
> recently. It catches a bug that's been going unnoticed. The `array[0]`
> load of the test case ends up on the exit of the loop above it. After
> pre/main/post loops are created for this loop (the inner loop is
> optimized out), the load is at a region merging control from the just
> created pre/main/post loops but its control (as returned by
> `PhaseIdealLoop::get_ctrl()`) is not updated by pre/main/post loops
> creation code so it's still the exit projection of one of the
> loops. This causes the assert to fire (the load has bad control) when
> RC runs next in the same loop opts pass.
>
> The current logic for pre/main/post loops creation does update the
> control of nodes on an exit projection but only if the node is
> referenced from some other node in the loop body.
>
> I think a similar bug with a node assigned control at the exit but not
> pinned is likely to exist. So I doubt simply going over nodes pinned
> at exits during pre/main/post and updating their control is good
> enough. What I propose instead is to `lazy_replace()` the projection
> by the region so that, for all nodes that are assigned the exit as
> control, `get_ctrl()` will then return the region. It's not entirely
> straightforward to implement because the `lazy_replace()` mechanism
> assumes the control being replaced is dead which is not the case
> here. That's why in the patch the exit projection is cloned so the
> existing one is dead and can be used in `lazy_replace()`. This also
> makes the calls to `sink_use()` redundant.
This looks good to me but needs some more comments.
src/hotspot/share/opto/loopTransform.cpp line 1921:
> 1919: post_head->set_post_loop(main_head);
> 1920:
> 1921: main_exit = outer_main_end->proj_out(false);
Please add a comment explaining that/why the `main_exit` projection changed.
src/hotspot/share/opto/loopopts.cpp line 2228:
> 2226:
> 2227: // If 'use' was in the loop-exit block, it now needs to be sunk
> 2228: // below the post-loop merge point.
This comment needs to be moved to the new lazy-replace logic and cloning needs to be explained there as well.
src/hotspot/share/opto/loopopts.cpp line 2616:
> 2614: }
> 2615:
> 2616: assert(use->is_Proj(), "loop exit should be projections");
Suggestion:
assert(use->is_Proj(), "loop exit should be projection");
src/hotspot/share/opto/loopopts.cpp line 2621:
> 2619: // Now finish up 'r'
> 2620: r->set_req(1, newuse);
> 2621: r->set_req(2, use_clone);
Suggestion:
r->set_req(2, use_clone);
src/hotspot/share/opto/loopopts.cpp line 2627:
> 2625: lazy_replace(use, r);
> 2626: // Map the (cloned) old use to the new merge point
> 2627: old_new.map( use_clone->_idx, r );
Suggestion:
old_new.map(use_clone->_idx, r);
-------------
Marked as reviewed by thartmann (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/15720#pullrequestreview-1628908813
PR Review Comment: https://git.openjdk.org/jdk/pull/15720#discussion_r1327258827
PR Review Comment: https://git.openjdk.org/jdk/pull/15720#discussion_r1327273206
PR Review Comment: https://git.openjdk.org/jdk/pull/15720#discussion_r1327263700
PR Review Comment: https://git.openjdk.org/jdk/pull/15720#discussion_r1327259375
PR Review Comment: https://git.openjdk.org/jdk/pull/15720#discussion_r1327264022
More information about the hotspot-compiler-dev
mailing list