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

[Christian Hagedorn]

On Tue, 22 Oct 2024 08:27:10 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.
>
> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update src/hotspot/share/opto/loopopts.cpp
>   
>   Co-authored-by: Tobias Hartmann <tobias.hartmann at oracle.com>

Looks good to me, too.

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21601#pullrequestreview-2384858522