8276064: CheckCastPP with raw oop input floats below a safepoint

[TobiHartmann]

This bug is similar to 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 is incomplete, because it only bails out of PhaseIdealLoop::try_sink_out_of_loop 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 did in GCM:

jdk/src/hotspot/share/opto/gcm.cpp

Lines 1325 to 1330 in 37107fc

	case Op_CheckCastPP: {
	// Don't move CheckCastPP nodes away from their input, if the input
	// is a rawptr (5071820).
	Node *def = self->in(1);
	if (def != NULL && def->bottom_type()->base() == Type::RawPtr) {
	early->add_inst(self);

The fix for JDK-8271600 can then be reverted, also because Roland's fix for JDK-8272562 disabled moving all CheckCastPPs out of loops anyway. Roland said that he plans to revisit that decision with 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.

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

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8276064: CheckCastPP with raw oop input floats below a safepoint

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer)
• Vladimir Ivanov (@iwanowww - Reviewer)
• Roland Westrelin (@rwestrel - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10932/head:pull/10932
$ git checkout pull/10932

Update a local copy of the PR:
$ git checkout pull/10932
$ git pull https://git.openjdk.org/jdk pull/10932/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10932

View PR using the GUI difftool:
$ git pr show -t 10932

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10932.diff

[bridgekeeper]

👋 Welcome back thartmann! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@TobiHartmann The following label will be automatically applied to this pull request:

• hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 00: Full (1ec3320b)

[vnkozlov]

Good and conservative solution.

Thanks for discussion offline with Tobias and Vladimir I. about this special case from Vector API.

[openjdk]

@TobiHartmann This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8276064: CheckCastPP with raw oop input floats below a safepoint

Reviewed-by: kvn, vlivanov, roland

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 150 new commits pushed to the master branch:

• 8eb90e2: 8296797: java/nio/channels/vthread/BlockingChannelOps.testSocketChannelWriteAsyncClose failed with ClosedChannelException
• a2cdcdd: 8296630: Fix SkipIfEqual on AArch64 and RISC-V
• 657a0b2: 8295865: Several issues with os::realloc
• ff2c987: 8294378: URLPermission constructor exception when using tr locale
• 34a499d: 8294033: x86_64: libm stubs are missing
• f0b648b: 8296758: [BACKOUT] Revert 8296115
• 7f587e5: 8296872: gtest is built with the build-jdk
• 819c691: 8295867: TestVerifyGraphEdges.java fails with exit code -1073741571 when using AlwaysIncrementalInline
• ced88a2: 8296733: JFR: File Read event for RandomAccessFile::write(byte[]) is incorrect
• 87b809a: 8296229: JFR: jfr tool should print unsigned values correctly
• ... and 140 more: https://git.openjdk.org/jdk/compare/7e88209e6c28ce18974308382948555f7c524721...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[iwanowww]

Looks good.

Thanks for fixing the issue!

[TobiHartmann]

Thanks for the reviews, @vnkozlov and @iwanowww! I'll run some performance testing, just to make sure.

[rwestrel]

Looks good to me.

[TobiHartmann]

Thanks, Roland!

[TobiHartmann]

/integrate

[openjdk]

Going to push as commit cd9c688.
Since your change was applied there have been 223 commits pushed to the master branch:

• d61720a: 8218885: Restore pop_frame and force_early_return functionality for Graal
• dd9aa72: 8296083: javax/swing/JTree/6263446/bug6263446.java fails intermittently on a VM
• cc44419: 8295407: C2 crash: Error: ShouldNotReachHere() in multiple vector tests with -XX:-MonomorphicArrayCheck -XX:-UncommonNullCast
• e2269fd: 8296968: Update langtools tests to use @enablePreview
• 2159170: 8296453: Simplify resource_area uses in ClassPathDirEntry::open_stream
• 95c390e: 8296956: [JVMCI] HotSpotResolvedJavaFieldImpl.getIndex returns wrong value
• 68d3ed5: 8296442: EncryptedPrivateKeyInfo can be created with an uninitialized AlgorithmParameters
• 37848a9: 8296967: [JVMCI] rationalize relationship between getCodeSize and getCode in ResolvedJavaMethod
• b3ef337: 8296960: [JVMCI] list HotSpotConstantPool.loadReferencedType to ConstantPool
• f0474b8: 8283238: make/scripts/compare.sh should show the diff when classlist does not match
• ... and 213 more: https://git.openjdk.org/jdk/compare/7e88209e6c28ce18974308382948555f7c524721...master

Your commit was automatically rebased without conflicts.

[openjdk]

@TobiHartmann Pushed as commit cd9c688.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.