8369805: [lworld] C2: assert(is_InlineType()) failed: invalid node class: Con

[chhagedorn]

TestLWorld.java was failing intermittently with -XX:+StressIGVN and -XX:+StressLoopPeeling due to not handling top correctly in InlineTypeNode::merge_with() during IGVN.

We have the following graph during IGVN:

The following steps happen:

1. 6837 Phi was created in Loop Peeling to merge data nodes from the peeled iteration and the remaining loop (this is the reason we are only seeing this failure with StressLoopPeeling - there are simply more chances with more phis to get unlucky and hit the assert).
2. When processing 6837 Phi in IGVN, we apply PhiNode::try_push_inline_types_down() in order to push the InlineType nodes down recursively.
3. Doing it recursively, we will then call PhiNode::try_push_inline_types_down() on 6535 Phi.
4. We create a new initial InlineType node based on the same klass as 6259 InlineType (i.e. the same fields) but use Phis for the inputs or InlineType nodes if the fields are value objects themselves:
   

   valhalla/src/hotspot/share/opto/inlinetypenode.cpp

   Lines 87 to 89 in abc1d5b

   	if (type->is_inlinetype() && no_circularity) {
   	// Handle inline type fields recursively
   	value = value->as_InlineType()->clone_with_phis(gvn, region, map);

   
   This new InlineType node represents the resulting "pushed down" InlineType. Let's call it inline-type-result.
5. We now populate the phi inputs of inline-type-result by visiting the InlineType inputs of 6535 Phi.
6. We visit the first input 6259 InlineType: We update the first input of the phis of inline-type-result by calling merge_with() on inline-type-result.
7. We visit all inputs of inline-type-result: These are either phis or InlineType nodes (see step 4). When we see an InlineType input, we must also see an InlineType input for 6259 InlineType because we used the same klass in step 4. However, we miss here that we could also have a top input because the node is actually being removed in IGVN because we only take the other path of 6535 Phi. Therefore this assumption is wrong:
   

   valhalla/src/hotspot/share/opto/inlinetypenode.cpp

   Lines 157 to 161 in abc1d5b

   	if (val1->is_InlineType()) {
   	if (val2->is_Phi()) {
   	val2 = gvn->transform(val2);
   	}
   	val1->as_InlineType()->merge_with(gvn, val2->as_InlineType(), pnum, transform);

   
   and we crash because val2 is top.

The fix is straight forward to stop the merge since the node is dead. Since this is highly intermittent and occurring with TestLWorld.java only, I have not added a separate test case. Additionally to standard testing, I also ran TestLWorld.java 100 times in the CI with the triggering flags without any failure anymore.

Thanks,
Christian

---

Progress

• Change must not contain extraneous whitespace

Issue

• JDK-8369805: [lworld] C2: assert(is_InlineType()) failed: invalid node class: Con (Bug - P4)

Reviewers

• Tobias Hartmann (@TobiHartmann - Committer) ⚠️ Review applies to 232e24f5

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1694

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1694.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

[openjdk]

@chhagedorn 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:

8369805: [lworld] C2: assert(is_InlineType()) failed: invalid node class: Con

Reviewed-by: thartmann

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 233 new commits pushed to the lworld branch:

• 412ec88: Merge jdk
• 5251405: 8368848: JShell's code completion not always working for multi-snippet inputs
• fa3af82: 8338197: [ubsan] ad_x86.hpp:6417:11: runtime error: shift exponent 100 is too large for 32-bit type 'unsigned int'
• ... and 230 more: https://git.openjdk.org/valhalla/compare/1db3cbe555a66327cfa4befb2a406ae2ac3b68e4...lworld

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 lworld branch, type /integrate in a new comment.

[mlbridge]

Webrevs

• 01: Full - Incremental (f03b789b)
• 00: Full (232e24f5)

[TobiHartmann]

Thanks for the thorough analysis Christian! Looks good to me.

[chhagedorn]

Thanks Tobias for your review!

/integrate

[openjdk]

Going to push as commit 60af17f.
Since your change was applied there have been 233 commits pushed to the lworld branch:

• 412ec88: Merge jdk
• 5251405: 8368848: JShell's code completion not always working for multi-snippet inputs
• fa3af82: 8338197: [ubsan] ad_x86.hpp:6417:11: runtime error: shift exponent 100 is too large for 32-bit type 'unsigned int'
• ... and 230 more: https://git.openjdk.org/valhalla/compare/1db3cbe555a66327cfa4befb2a406ae2ac3b68e4...lworld

Your commit was automatically rebased without conflicts.

[openjdk]

@chhagedorn Pushed as commit 60af17f.

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