RFR: 8370332: C2 SuperWord: SIGSEGV because PhaseIdealLoop::split_thru_phi left dead nodes in loop _body [v2]

[Christian Hagedorn]

On Tue, 28 Oct 2025 16:30:24 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Analysis:
>> `split_thru_phi` can split a node out of the loop, through some loop phi. As a consequence, that node and the phi we split through can become dead. But `split_thru_phi` did not have any logic to yank the dead node and phi from the `_body`. If this happens in the same loop-opts-phase as a later SuperWord, and that SuperWord pass somehow accesses that loop `_body`, then we may find dead nodes, which is not expected.
>> 
>> It is not ok that `split_thru_phi` leaves dead nodes in the `_body`, so they have to be yanked.
>> 
>> What I did additionally: I went through all uses of `split_thru_phi`, and moved the `replace_node` from the call-site to the method itself. Removing the node and yanking from `_body` conceptually belongs together, so they should be together in code.
>> 
>> I suspect that `split_thru_phi` was broken for a long time already. But JDK26 changes in SuperWord started to check inputs of all nodes in `_body`, and that fails with dead nodes.
>> 
>> Future Work:
>> - Continue work on making `VerifyLoopOptimizations` work again, we should assert that there are no dead nodes in the `_body`. We may do that with the following task, or a subsequent one.
>>   - [JDK-8370332](https://bugs.openjdk.org/browse/JDK-8370332) Fix VerifyLoopOptimizations - step 3 - fix ctrl/loop
>
> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
> 
>   allow unique out with multiple uses

That looks good to me and I agree to add verification for that case with `VerifyLoopOptimizations` at some point.

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

> 233:   // just split through now has no use any more, it also
> 234:   // has to be removed.
> 235:   IdealLoopTree* region_loop = get_loop(region);

The method is already quite large. This could probably nicely be extracted to a "yank_old_nodes()" method or something like that. You can keep the ` _igvn.replace_node(n, phi)` in this method.

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

> 234:   // has to be removed.
> 235:   IdealLoopTree* region_loop = get_loop(region);
> 236:   if (region->is_Loop() && region_loop->_child == nullptr) {

I think you can use `region_loop->is_innermost()` instead:

Suggestion:

  if (region->is_Loop() && region_loop->is_innermost()) {

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/27955#pullrequestreview-3410062237
PR Review Comment: https://git.openjdk.org/jdk/pull/27955#discussion_r2485788488
PR Review Comment: https://git.openjdk.org/jdk/pull/27955#discussion_r2485773527