RFR: 8342330: C2: "node pinned on loop exit test?" assert failure

[Tobias Hartmann]

On Mon, 21 Oct 2024 08:40:31 GMT, Roland Westrelin <roland at openjdk.org> wrote:

> The assert fires because range check elimination processes a test of
> the shape:
> 
> 
> if (i * 4 != (x - objectField.intField) - 1)) {
>   ...
> }
> 
> 
> and `(x - objectField.intField) - 1)` has control on the exit
> projection of the pre loop.
> 
> This happens because:
> 
> - `objectField.intField` depends on the null check of `objectField`
>   which is performed in the pre loop.
>   
> - `i * scale + (objectField.intField + 1) == x` is transformed into:
>   `i * scale == x - (objectField.intField + 1)`
> 
> - `(x - objectField.intField) - 1)` only has uses out of the pre loop
>   and is sunk out of the loop. It ends up pinned on the the exit
>   projection of the pre loop.
>   
> 
> There is already logic in `PhaseIdealLoop::ctrl_of_use_out_of_loop()`
> to handle similar cases but, here, the difference is that the use
> (`SubI` of 1) for what's being sunk doesn't have control in the main
> loop but between the pre and main loop so that logic doesn't catch
> this case.
> 
> There is also a possible bug in that logic:
> 
> 
> n_loop->_next == get_loop(u_loop->_head->as_CountedLoop()->skip_strip_mined())
> 
> 
> assumes the loop that follows the pre loop in the loop tree is the
> main loop which is not guaranteed.
> 
> In this particular case, the assert is harmless: RCE can't eliminate
> the condition but it's hard to rule out a similar scenario with a
> condition that RCE could remove. I propose revisiting the condition in
> `PhaseIdealLoop::ctrl_of_use_out_of_loop()` so it skips all uses that
> are dominated by the loop exit of the pre loop.

Looks reasonable to me.

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

> 1947:   // test of the pre loop above the point in the graph where it's pinned.
> 1948:   if (n_loop->_head->is_CountedLoop() && n_loop->_head->as_CountedLoop()->is_pre_loop()) {
> 1949:     bool res = false;

Suggestion:

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

Marked as reviewed by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21601#pullrequestreview-2384198539
PR Review Comment: https://git.openjdk.org/jdk/pull/21601#discussion_r1810120394