RFR: 8276064: CheckCastPP with raw oop input floats below a safepoint
Vladimir Ivanov
vlivanov at openjdk.org
Tue Nov 1 17:44:26 UTC 2022
On Tue, 1 Nov 2022 11:55:04 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:
> This bug is similar to [JDK-8271600](https://bugs.openjdk.org/browse/JDK-8271600): A CheckCastPP with a raw oop input floats out of a loop and below a safepoint. Since C2 does not generate OopMap entries for raw pointers, the GC will not update the oop if the corresponding object is moved during the safepoint. We either assert already during OopMap creation, or crash when dereferencing a stale oop during runtime (the verification code does not always detect such live raw oops at safepoints, I included a fix for that as well).
>
> I think the fix for [JDK-8271600](https://bugs.openjdk.org/browse/JDK-8271600) is incomplete, because it only bails out of [PhaseIdealLoop::try_sink_out_of_loop](https://github.com/openjdk/jdk/commit/2ff4c01d42f1afcc53abd48e074356fb4a700754) while the underlying issue is that a raw CheckCastPP ends up with ctrl "far away" from its Allocate/Initialize and potentially even below a safepoint. Usually, the CheckCastPP would always be part of safepoint debug info and therefore late ctrl would be guaranteed to be above the safepoint. However, vector objects are aggressively scalar replaced in safepoints, which allows late ctrl to be set to further below. This is specific to vectors, since "normal" Java objects would either be fully scalarized or not be scalarized at all.
>
> In the failing case, Loop Unswitching clones the loop body and creates a Phi to merge the oop results from the vector allocations in both loops. Since ctrl of the CheckCastPP is outside of the loop, its data input is changed to the newly created Phi and its control input is set to the region that merges the loop exits. This moves the CheckCastPP below a safepoint in the loop.
>
> Below graphs show the details. `395 CheckCastPP` is removed from the debug info for `262 CallStaticJava` because it's scalarized (`326 SafepointScalarObject`). Late ctrl is then computed to be outside of the loop because the CheckCastPP is only used in the return.
>
> 
>
> Now Loop Unswitching creates a `487 Region` and `517 Phi` to merge control and data inputs to the CheckCastPP from the fast and slow loops (see `PhaseIdealLoop::clone_loop_handle_data_uses`). Control of the `395 CheckCastPP` is updated accordingly.
>
> 
>
> As a result, the raw oop input of the `395 CheckCastPP` is live at `262 CallStaticJava`.
>
> We could now add another point fix to prevent loop unswitching from moving the CheckCastPP out of the loop, but I think there is a risk that other current or future optimizations would rely on the CheckCastPP's late ctrl and do a similar thing. I would therefore suggest to pin all CheckCastPPs with a raw oop input, similar to what [JDK-5071820](https://bugs.openjdk.org/browse/JDK-5071820) did in GCM:
> https://github.com/openjdk/jdk/blob/37107fc1574a4191987420d88f7182e63c7da60c/src/hotspot/share/opto/gcm.cpp#L1325-L1330
>
> The fix for [JDK-8271600](https://bugs.openjdk.org/browse/JDK-8271600) can then be reverted, also because Roland's fix for [JDK-8272562](https://bugs.openjdk.org/browse/JDK-8272562) disabled moving **all** CheckCastPPs out of loops anyway. Roland said that he plans to revisit that decision with [JDK-8275202](https://bugs.openjdk.org/browse/JDK-8275202). The tests added with this PR will cover the `PhaseIdealLoop::try_sink_out_of_loop` case as well and therefore serve as regression tests for [JDK-8271600](https://bugs.openjdk.org/browse/JDK-8271600).
>
> We could improve this by adding logic to set late ctrl just above the safepoint, but I'm not sure if it's worth the complexity because we would need to walk up the control paths from late to early control and compute the dominator of all safepoints.
>
> I also fixed the verification code in `OopFlow::build_oop_map` to account for spilling. Before, compilation of `test1` would pass and only crash during execution. Now, we assert and print:
>
>
> 454 DefinitionSpillCopy === _ 122 [[ 321 ]] !jvms: IntVector::zero @ bci:26 (line 563) TestRawOopAtSafepoint::test1 @ bci:25 (line 74)
> 321 Phi === 315 454 503 [[ 512 ]] #rawptr:NotNull !jvms: IntVector::zero @ bci:26 (line 563) TestRawOopAtSafepoint::test1 @ bci:25 (line 74)
> 38 CallStaticJavaDirect === 40 126 136 102 0 455 452 138 139 453 460 [[ 39 84 130 37 388 ]] Static compiler.vectorapi.TestRawOopAtSafepoint::safepoint # void ( int ) TestRawOopAtSafepoint::test1 @ bci:44 (line 75) !jvms: TestRawOopAtSafepoint::test1 @ bci:44 (line 75)
>
>
>
> What do you think?
>
> Thanks,
> Tobias
Looks good.
Thanks for fixing the issue!
-------------
Marked as reviewed by vlivanov (Reviewer).
PR: https://git.openjdk.org/jdk/pull/10932
More information about the hotspot-compiler-dev
mailing list