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

[Qizheng Xing]

On Tue, 18 Mar 2025 08:03:57 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Hi all,
>> 
>> This patch has now passed all GHA tests and is ready for further reviews.
>> 
>> If there are any other suggestions for this PR, please let me know.
>> 
>> Thanks!
>
> @MaxXSoft I'm not an expert with SafePoints, but I'd be willing to review if you answer my questions above, and maybe some more I'll have later ;)
> 
> One question I just had now: Assume we now remove the SafePoint because there is that other call above. But what if later we inline that call - do we still have some SafePoint after that in the loop?

Hi @eme64 ,

Thanks for your previous reviews! Regarding your first question:

> Though I am struggling to understand all of the comments...
>
> is that talking about a call in the outer loop, or the inner loop?

This comment is really confusing. I had to review the code in this file multiple times to understand it properly. Here's my understanding:

If there's a call in a loop that will definitely be executed and will perform a safepoint poll (`guaranteed_safepoint`), then we can remove all other non-call safepoints except for this call.

However, in some cases, a non-call safepoint (ncsfpt) removed from an inner loop might also be part of an outer loop. If this removed safepoint happens to be the only one in the outer loop, it could cause problems. We should avoid this situation.

The purpose of this method is to examine all loops in the loop tree, and mark ncsfpts that shouldn't be removed. Note that this method DOESN'T actually remove any safepoints; it only marks them. The actual removal will happen later, checking whether each safepoint has been marked by outer loops before deletion.

The `C)` mentioned in the comment refers to: if a loop already contains a call that will definitely be executed, there's no need to mark any other safepoints. This is because no matter how many safepoints are removed from its inner loops, even if those safepoints are part of the outer loop, that call will still perform a safepoint poll.

Therefore, the conclusion is that the call mentioned in `C)` refers to a call in the outer loop.

The third question:

> Assume we now remove the SafePoint because there is that other call above. But what if later we inline that call - do we still have some SafePoint after that in the loop?

On the one hand, this situation won't occur in the current `Compile::Optimize` process. The `Optimize` method will always complete all inlining before performing loop optimization, as seen in:

https://github.com/openjdk/jdk/blob/3d3b7820371058b40f2e694536c98aa3900abb5f/src/hotspot/share/opto/compile.cpp#L2363-L2365

On the other hand, this patch only fixes the issue where phase ideal loop cannot remove redundant ncsfpts in certain situations. In other cases, such as when ncsfpt appears before a call, C2 can still remove the ncsfpt even without this patch, resulting in a loop with no safepoints except for the call. If the issue you mentioned exists, this risk should have been present before, but so far, we haven't encountered this problem in any test or real-world scenarios.

That's all. I'm still working on the second question and will add a comment once I figure it out.

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

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