RFR: 8364757: Missing Store nodes caused by bad wiring in PhaseIdealLoop::insert_post_loop [v11]

[Roland Westrelin]

On Tue, 30 Sep 2025 07:19:04 GMT, Benoît Maillard <bmaillard at openjdk.org> wrote:

>> This PR introduces a fix for wrong results caused by missing `Store` nodes in C2 IR due to incorrect wiring in `PhaseIdealLoop::insert_post_loop`.
>> 
>> ### Context
>> 
>> The issue was initially found by the fuzzer. After some trial and error, and with the help of @chhagedorn I was able to reduce the reproducer to something very simple. After being compiled by C2, the execution of the following method led to the last statement (`x = 0`) to be ignored:
>> 
>> 
>>     static public void test() {
>>         x = 0;
>>         for (int i = 0; i < 20000; i++) {
>>             x += i;
>>         }
>>         x = 0;
>>     }
>> 
>> 
>> After some investigation and discussions with @robcasloz and @chhagedorn, it appeared that this issue is linked to how safepoints are inserted into long running loops, causing the loop to be transformed into a nested loop with an `OuterStripMinedLoop` node. `Store` node are moved out of the inner loop when encountering this pattern, and the associated `Phi` nodes are removed in order to avoid inhibiting loop optimizations taking place later. This was initially adressed in [JDK-8356708](https://bugs.openjdk.org/browse/JDK-8356708) by making the necessary corrections in macro expansion. As explained in the next section, this is not enough here as macro expansion happens too late.
>> 
>> This PR aims at addressing the specific case of the wrong wiring of `Store` nodes in _post_ loops, but on the longer term further investigations into the missing `Phi` node issue are necessary, as they are likely to cause other issues (cf. related JBS issues).
>> 
>> ### Detailed Analysis
>> 
>> In `PhaseIdealLoop::create_outer_strip_mined_loop`, a simple `CountedLoop` is turned into a nested loop with an `OuterStripMinedLoop`. The body of the initial loop remains in the inner loop, but the safepoint is moved to the outer loop. Later, we attempt to move `Store` nodes after the inner loop in `PhaseIdealLoop::try_move_store_after_loop`.  When the `Store` node is moved to the outer loop, we also get rid of its input `Phi` node in order not to confuse loop optimizations happening later.
>> 
>> This only becomes a problem in `PhaseIdealLoop::insert_post_loop`, where we clone the body of the inner/outer loop for the iterations remaining after unrolling. There, we use `Phi` nodes to do the necessary rewiring between the original body and the cloned one. Because we do not have `Phi` nodes for the moved `Store` nodes, their memory inputs may end up being incorrect.
>> 
>> This is wh...
>
> Benoît Maillard has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix bad github commit suggestion

Other than that, looks good to me.

src/hotspot/share/opto/loopTransform.cpp line 1686:

> 1684:       if (use->is_Store() && use->in(MemNode::Memory) == last) {
> 1685:         IdealLoopTree* use_loop = get_loop(get_ctrl(use));
> 1686:         if (outer_loop->is_member(use_loop)) {

You could use `PhaseIdealLoop::is_member()` here. Not need for the `get_loop()` call then.

src/hotspot/share/opto/loopTransform.cpp line 1800:

> 1798:       // body was cloned as a unit
> 1799:       IdealLoopTree* input_loop = get_loop(get_ctrl(store->in(MemNode::Memory)));
> 1800:       if (!outer_loop->is_member(input_loop)) {

Same here. Actually, I wonder if a new method that also does the `get_ctrl()` (or `ctrl_or_self()`), wouldn't be useful given that pattern must be quite common.

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

PR Review: https://git.openjdk.org/jdk/pull/27225#pullrequestreview-3285125030
PR Review Comment: https://git.openjdk.org/jdk/pull/27225#discussion_r2391678560
PR Review Comment: https://git.openjdk.org/jdk/pull/27225#discussion_r2391684977