8387488: [lworld] C2: acmp expansion fails with assert(replace != nullptr) failed: must succeed

[TobiHartmann]

When slightly modifying the test that I added for JDK-8386067, I hit an assert in CallStaticJavaNode::replace_is_substitutable because expansion of the substitutability call failed and emit_substitutability_check returned nullptr although can_emit_substitutability_check returned true.

In the new test, the problematic field is ObjectHolder::obj which, although it is of type Object, is always exact and therefore can't be a value object and does not require a substitutability test. InlineTypeNode::can_emit_substitutability_check correctly returns true:

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

Lines 611 to 614 in 2ae8e47

	if (!lhs->bottom_type()->is_ptr()->can_be_inline_type() ||
	(rhs != nullptr && !rhs->bottom_type()->is_ptr()->can_be_inline_type())) {
	return true;
	}

However, during recursive expansion, nullable value object fields are null-checked and then compared field-by-field:

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

Line 910 in 2ae8e47

cur_lhs_field = kit->null_check_common(cur_lhs_field, T_OBJECT, false, &null_unknown);

The code unconditionally rebuilt those fields with InlineTypeNode::make_from_oop after the null and type checks because there's a CastPP in-between:

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

Lines 955 to 960 in 2ae8e47

	cur_lhs_field = kit->gen_checkcast(cur_lhs_field, vk_klass, &not_vk);
	if (!not_vk->is_top()) {
	region->add_req(not_vk);
	result->add_req(igvn.intcon(0));
	}
	cur_lhs_field = InlineTypeNode::make_from_oop(kit, cur_lhs_field, vk);

If the field was already scalarized, this discarded the existing InlineTypeNode and reloaded fields from the oop, losing precise information such as exact Object field Phis. The later Object field comparison then looks non-exact and emit_substitutability_check_pointer returns nullptr:

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

Lines 722 to 724 in 2ae8e47

	if (!lhs_type->is_inlinetypeptr() && !rhs_type->is_inlinetypeptr()) {
	return nullptr;
	}

The fix is to preserve already-scalarized recursive field operands when their inline klass matches the comparison klass, and only checkcast/reload operands that are not scalarized. With the fix, exact type information is preserved and we emit a pointer comparison for the exact object fields:

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

Lines 700 to 702 in 2ae8e47

	} else if (!lhs_type->is_ptr()->can_be_inline_type() || !rhs_type->is_ptr()->can_be_inline_type()) {
	// If one of the sides is not a value object, can only be substitutable if they are the same
	cmp = kit->CmpP(lhs, rhs);

The IR Framework test verifies this successful expansion.

Note: The null and type checks are not immediately folded because by GVN because we need to set set_delay_transform here:

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

Lines 1412 to 1416 in 2ae8e47

	igvn->set_delay_transform(true);
	GraphKit kit(this, *igvn);
	Node* replace = InlineTypeNode::emit_substitutability_check(&kit, left, right);
	igvn->set_delay_transform(false);

Thanks,
Tobias

---

• I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

---

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-8387488: [lworld] C2: acmp expansion fails with assert(replace != nullptr) failed: must succeed (Bug - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2602

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[TobiHartmann]

Just found this missing TODO when browsing code.

[mlbridge]

Webrevs

• 01: Full - Incremental (1016b748)
• 00: Full (6adb8936)

[merykitty]

You can add a comment that it is not an issue if cur_lhs_inline is nullable and cur_lhs_field is null-checked, it is null-checked so that we can load the fields, and an InlineTypeNode does that already.

[merykitty]

It's not quite what I want to say. The null check is still present, the thing is that we ignored the null-checked value (the CastPP) and just use the raw value. This is because normally, we need a non-null CastPP to load from the object. That is unnecessary for an InlineTypeNode because we don't perform the loads.

[merykitty]

It would probably be better to name this more specific to its type name so as not to have name collision.

[marc-chevalier]

Haven't found anything crazy, but with low confidence. Happy @merykitty was faster than me at reviewing this.

[TobiHartmann]

Thanks for the reviews Quan Anh and Marc! @merykitty I added a comment and adjusted the field name in the test. Please let me know what you think.