RFR: 8347499: C2: Make `PhaseIdealLoop` eliminate more redundant safepoints in loops [v3]

Tue Sep 16 06:20:26 UTC 2025

On Thu, 22 May 2025 07:53:39 GMT, Qizheng Xing <qxing at openjdk.org> wrote:

>> In `PhaseIdealLoop`, `IdealLoopTree::check_safepts` method checks if any call that is guaranteed to have a safepoint dominates the tail of the loop. In the previous implementation, `check_safepts` would stop if it found a local non-call safepoint. At this time, if there was a call before the safepoint in the dom-path, this safepoint would not be eliminated.
>> 
>> <img width="353" alt="loop-safepoint" src="https://github.com/user-attachments/assets/c220e103-aaba-4e3f-98ac-1ddb6465c309" />
>> 
>> This patch changes the behavior of `check_safepts` to not stop when it finds a non-local safepoint. This makes simple loops with one method call ~3.8% faster (on aarch64).
>> 
>> 
>> Benchmark                Mode  Cnt       Score      Error  Units
>> LoopSafepoint.loopVar    avgt   15  208296.259 ± 1350.409  ns/op   # baseline
>> LoopSafepoint.loopVar    avgt   15  200692.874 ±  616.770  ns/op   # this patch
>> 
>> 
>> Testing: tier1-2 on x86_64 and aarch64.
>
> Qizheng Xing has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Improve documentation comments

Wow, this took me way too long to have a look at.
But I feel like now I understand what's going on, so thanks for the additional changes on the documentation.

The approach seems very reasonable. I'd lilke to see a few more tests though. Maybe you can instead point me to tests that already exist, that would be fine too.

I'm soon going on vacation, so it may take me even more time to re-review.
But I'd suggest that @rwestrel look at this PR, since he last worked on this code.

@MaxXSoft Can you please merge with master as well?

I think we should also run some larger benchmarking on this patch, just to see if there are any surprises (I'd expect mostly improvements, but we shall see).

src/hotspot/share/opto/loopnode.cpp line 3818:

> 3816: //               /   |  |
> 3817: //              v    +--+
> 3818: //        exit  4

This drawing seems a bit confusing. There seem to be 3 edges coming out of 2.
Do you think you could fix it too, just to create more clarity in the code?

src/hotspot/share/opto/loopnode.cpp line 3830:

> 3828: //
> 3829: // The insights into the problem:
> 3830: //  A) Counted loops are okay

What does it mean to be "okay"? Why are they "okay"?

src/hotspot/share/opto/loopnode.cpp line 3832:

> 3830: //  A) Counted loops are okay
> 3831: //  B) Innermost loops are okay because there's no inner loops can delete
> 3832: //     their ncsfpts

Suggestion:

//  B) Innermost loops are okay because there's no inner loops that can
//     delete their ncsfpts

Missing `that`. I feel that we are now losing information. The previous comment made a promise that your comment does not make any more. Is that intentional?

It seems the logic was: only outer loops need to mark safepoints for protection, because only loops further in can remove safepoints. Is that still correct?

src/hotspot/share/opto/loopnode.cpp line 3840:

> 3838: //     inside any nested loop, then that loop is okay
> 3839: //  E) Otherwise, if an outer loop's ncsfpt on the idom-path is nested in
> 3840: //     an inner loop, we need to prevent the inner loop from deleting it

Nice, that's indeed an improvement :)

test/hotspot/jtreg/compiler/c2/irTests/TestLoopSafepoint.java line 24:

> 22:  */
> 23: 
> 24: package compiler.c2.irTests;

We'd like to get away from putting all IR tests in `irTests`, and we'd rather put them into thematic directories.
Proposal: `compiler/loopopts/TestRedundantSafePointElimination.java`

test/hotspot/jtreg/compiler/c2/irTests/TestLoopSafepoint.java line 33:

> 31:  * @summary Tests that redundant safepoints can be eliminated in loops.
> 32:  * @library /test/lib /
> 33:  * @requires vm.compiler2.enabled

Is this `@requires` strictly required? If not, remove it so we can run these tests also with C1 and other compilers.

test/hotspot/jtreg/compiler/c2/irTests/TestLoopSafepoint.java line 66:

> 64:             empty();
> 65:         }
> 66:     }

So these do not end up being CountedLoop?

test/hotspot/jtreg/compiler/c2/irTests/TestLoopSafepoint.java line 84:

> 82:             empty();
> 83:         }
> 84:     }

All of the cases here are only single loops, right? But is the algorithm not mostly dealing with nested loops, where we have to make sure that in some cases the `SafePoint` is not eliminated? Could you add some extra cases for that?

test/micro/org/openjdk/bench/vm/compiler/LoopSafepoint.java line 76:

> 74:         }
> 75:         return sum;
> 76:     }

I think it would be nice if you made the examples in the JMH and the JTREG as similar as possible.

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23057#pullrequestreview-3227461101
PR Review Comment: https://git.openjdk.org/jdk/pull/23057#discussion_r2350903845
PR Review Comment: https://git.openjdk.org/jdk/pull/23057#discussion_r2350907448
PR Review Comment: https://git.openjdk.org/jdk/pull/23057#discussion_r2350916805
PR Review Comment: https://git.openjdk.org/jdk/pull/23057#discussion_r2350922211
PR Review Comment: https://git.openjdk.org/jdk/pull/23057#discussion_r2350951501
PR Review Comment: https://git.openjdk.org/jdk/pull/23057#discussion_r2350952358
PR Review Comment: https://git.openjdk.org/jdk/pull/23057#discussion_r2350961237
PR Review Comment: https://git.openjdk.org/jdk/pull/23057#discussion_r2350965087
PR Review Comment: https://git.openjdk.org/jdk/pull/23057#discussion_r2350967582