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

[Qizheng Xing]

On Wed, 5 Feb 2025 09:00:38 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - Merge branch 'master' into enhance-loop-safepoint-elim
>  - Add IR test and microbench.
>  - Make `PhaseIdealLoop` eliminate more redundant safepoints in loops.

The second question:

> If we now removed safepoints in places where we would actually have needed them: how would we find out? I suppose we would get longer time to safepoint - higher latency in some cases. How would we catch this with our tests?

I tried running tier1 tests with `JAVA_OPTIONS=-XX:+UnlockDiagnosticVMOptions -XX:+SafepointTimeout -XX:+AbortVMOnSafepointTimeout -XX:SafepointTimeoutDelay=1000`, and there were no failures.

Running with `-XX:SafepointTimeoutDelay=500` caused 1 random JDK test case to fail. But then I tried to build a JDK without this patch, and it still had the random failure with this option.

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

PR Comment: https://git.openjdk.org/jdk/pull/23057#issuecomment-2753652662