RFR: 8260420: C2 compilation fails with assert(found_sfpt) failed: no node in loop that's not input to safepoint [v2]

[Vladimir Kozlov]

On Fri, 29 Jan 2021 14:59:14 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

>> Loop strip mining verification fails because a `LoadNode` with no safepoint use ends up in the `OuterStripMinedLoop`. The root cause is very similar to [JDK-8249607](https://bugs.openjdk.java.net/browse/JDK-8249607): A `LoadNode` with two uses (a field store and a return, see `test2`) is cloned by `PhaseIdealLoop::split_if_with_blocks_post()` for each use, to allow it to flow out of the loop. Both clones end up in the `OuterStripMinedLoop`, the clone for the field store because the store has a safepoint use and the clone for the return because `late_load_ctrl` is too conservative by taking all initial uses into account.
>> 
>> Now the load without a safepoint use is always detected by loop strip mining verification but without verification (for example, in a product build), we still hit several different issues depending on the exact use of the load (compare `test2/3/4`). The main issue is that loads without a safepoint use are not correctly wired when creating pre/main/post loops. Christian described this well in his RFR for [JDK-8249607](https://bugs.openjdk.java.net/browse/JDK-8249607): https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2020-August/039638.html
>> 
>> Therefore, relaxing loop strip mining verification is not an option.
>> 
>> The problem with the fix for [JDK-8249607](https://bugs.openjdk.java.net/browse/JDK-8249607) is that it relies on IGVN being executed before pre/main/post loops are created, merging the two clones such that the remaining one has a safepoint use and is therefore correctly handled. However, this is not guaranteed. In fact, if `major_progress` is not set, pre/main/post loops could be created in the same round of loop opts without IGVN in-between (`IdealLoopTree::iteration_split` is executed right after `PhaseIdealLoop::split_if_with_blocks`).
>> 
>> I think we have the following options:
>> 1) Bail out if the loop is strip mined. I think that would be too conservative.
>> 
>> 2) Simply set `major_progress` when pinning a `LoadNode` to the `OuterStripMinedLoop` to make sure IGVN is executed and duplicate `LoadNodes` are merged. That seems quite invasive though.
>> 
>> 3) Allow the cloned load without a safepoint use (and therefore no usages in the `OuterStripMinedLoop`) to completely flow out of the loop. Currently, this is blocked by `late_load_ctrl` being computed on the initial `LoadNode` instead of the clone and therefore also taking into account the store that is referenced by the safepoint. We could simply re-compute the late ctrl for the cloned load, allowing it to flow out of the `OuterStripMinedLoop`. However, that only works if there is no anti-dependency in the `OuterStripMinedLoop`.
>> 
>> 4) Detect duplicate loads in the `OuterStripMinedLoop` on creation in `PhaseIdealLoop::split_if_with_blocks` and merge them right away.
>> 
>> I've decided to go with 4) and also reverted the fix for JDK-8249607 which is no longer required.
>> 
>> As Roland and Christian already noticed, there are various other issues with that code that hopefully will be addressed at some point by [JDK-8252372](https://bugs.openjdk.java.net/browse/JDK-8252372).
>> 
>> Thanks,
>> Tobias
>
> Tobias Hartmann has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove late ctrl update

Good.

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

Marked as reviewed by kvn (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2315