8367623: [lworld] C2: Ideal Optimization on InlineTypeNode should not be carried out after Macro Expansion

[benoitmaillard]

This PR tightens the conditions under which an optimization from InlineTypeNode::Ideal should be carried out. This optimization was initially reported as missed with -XX:VerifyIterativeGVN.

Summary

The original failure appeared with TestFieldNullMarkers.java and -XX:VerifyIterativeGVN=1110. This test performs various allocations with value classes and other Valhalla features.

In InlineTypeNode::Ideal, we have the following optimization:

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

Lines 835 to 853 in b1d14c6

	// Use base oop if fields are loaded from memory, don't do so if base is the CheckCastPP of an
	// allocation because the only case we load from a naked CheckCastPP is when we exit a
	// constructor of an inline type and we want to relinquish the larval oop there. This has a
	// couple of benefits:
	// - The allocation is likely to be elided earlier if it is not an input of an InlineTypeNode.
	// - The InlineTypeNode without an allocation input is more likely to be GVN-ed. This may emerge
	// when we try to clone a value object.
	// - The buffering, if needed, is delayed until it is required. This new allocation, since it is
	// created from an InlineTypeNode, is recognized as not having a unique identity and in the
	// future, we can move them around more freely such as hoisting out of loops. This is not true
	// for the old allocation since larval value objects do have unique identities.
	Node* base = is_loaded(phase);
	if (base != nullptr && !base->is_InlineType() && !phase->type(base)->maybe_null() && AllocateNode::Ideal_allocation(base) == nullptr) {
	if (oop != base || phase->type(is_buffered) != TypeInt::ONE) {
	set_oop(*phase, base);
	set_is_buffered(*phase);
	return this;
	}
	}

As explained in the optimization comments, we don't want to use the base oop if it corresponds to the larval oop. This is enforced by the condition AllocateNode::Ideal_allocation(base) == nullptr.

In our case this is exactly what happens, and the optimization is prevented. HOwever, this changes during macro expansion. The allocate node disappears, and AllocateNode::Ideal_allocation(base) == nullptr becomes true. As this is not intended, there is no notification mechanism for this case and we end up with the missing optimization assert.

The conditions for this bug to reproduce are somewhat subtle. When things go well, the larval Allocate node is eliminated by eliminate_allocate_node before this problematic case shows up. However, there are cases where the <init> method is not inlined, and this prevents the removal of the Allocate node. It stays until macro expansion, and that is where things go wrong.

In the extracted reproducer, the <init> method is not inlined because of unloaded signature classes, as CompileCommand=printinlining shows:

CompileCommand: PrintInlining *.* bool PrintInlining = true
                            @ 5   compiler.valhalla.inlinetypes.TestMissingOptUseBaseOop$MyValue::<init> (10 bytes)   failed to inline: unloaded signature classes

This could also happen for other reasons though.

Solution

The solution is to not do this optimization after macro expansion. Adding a phase->C->allow_macro_nodes() check ensures that the AllocateNode::Ideal_allocation(base) call is relevant in the current phase and that we can use it to check if we are dealing with the larval oop.

Testing

• GitHub Actions
• tier1-4, plus some internal testing

---

Progress

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

Issue

• JDK-8367623: [lworld] C2: Ideal Optimization on InlineTypeNode should not be carried out after Macro Expansion (Bug - P3)

Reviewers

• Quan Anh Mai (@merykitty - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1742

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back bmaillard! 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]

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

8367623: [lworld] C2: Ideal Optimization on InlineTypeNode should not be carried out after Macro Expansion

Reviewed-by: qamai

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

• 0e99563: 8371826: [lworld] Migrated value classes pre-registered unconditionally
• ba0f2bb: 8372160: [lworld] Re-link ToDos from 8329224 to 8251971
• 51440e1: 8365895: [lworld] Four compiler tests on windows OOM after jdk-25+25 merge
• ... and 688 more: https://git.openjdk.org/valhalla/compare/e0aaf29064ed67c240f5275e9b3d324ce5550382...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@merykitty) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[mlbridge]

Webrevs

• 00: Full (43b36743)

[merykitty]

Maybe we should assert that AllocateNode::Ideal_allocation can only be called when C->allow_macro_nodes() so that the result is meaningful.