[lworld] RFR: 8228361: [lworld] Optimize the substitutability check in C2 [v3]

[Manuel Hässig]

On Thu, 18 Dec 2025 13:42:19 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

>> C2 will now remove the slow call to `ValueObjectMethods::isSubstitutable(Alt)` whenever it's able to determine the layout of one of the operands. It will then emit code to directly compare the fields.
>> 
>> This patch also contains an intrinsic for `_getFieldMap` that will be used by the new core-libs implementation of the substitutability check ([JDK-8370450](https://bugs.openjdk.org/browse/JDK-8370450)) that's used by the interpreter / C1 and as a slow path in C2.
>> 
>> When browsing code, I marked a few rough edges in unrelated code for follow-up cleanups with the corresponding bug numbers.
>> 
>> Testing: tier1-tier6 + valhalla-comp-stress
>> 
>> Thanks,
>> Tobias
>
> Tobias Hartmann has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Adjustments according to review comments

Thank you for this improvement, @TobiHartmann! The logic looks good to me. I mainly have some readability and factoring suggestions of the take it or leave it kind and a misunderstanding about a comment.

src/hotspot/share/opto/callnode.cpp line 1184:

> 1182:   }
> 1183: 
> 1184:   // Try to replace the runtime call to the substitutability test emitted by acmp if (at least) one operand is a known type

If this would be in its own helper method, `Ideal` would stay smaller and feel less about substitutability, and you could reduce the nesting with short circuit returns. Not sure if this is worth the plumbing though.

src/hotspot/share/opto/inlinetypenode.cpp line 675:

> 673: bool InlineTypeNode::can_emit_substitutability_check(Node* other) const {
> 674:   if (other != nullptr && other->is_InlineType() && bottom_type() != other->bottom_type()) {
> 675:     // Different types, this is dead code because there's a check above that guarantees this.

Shouldn't this be an assert, then? Or at least call it a sanity check instead, just so no one gets an idea and removes it when they have not realized yet that the bottom type check is essential for the rest of the method.

src/hotspot/share/opto/inlinetypenode.cpp line 694:

> 692: 
> 693: // Emit IR to check substitutability between 'this' (left operand) and the value object referred to by 'other' (right operand).
> 694: // Parse-time checks guarantee that both operands have the same type. If 'other' is not an InlineTypeNode, we need to emit loads for the field values.

If both `this` and `other` have the same type and `this` is an `InlineTypeNode`, how can `other` not be an `InlineTypeNode`?

src/hotspot/share/opto/inlinetypenode.cpp line 781:

> 779:     } else {
> 780:       assert(ft->is_primitive_type() || !ft->as_klass()->can_be_inline_klass(), "Needs substitutability test");
> 781:       acmp_val_guard(igvn, region, phi, ctrl, bt, BoolTest::ne, this_field, other_field);

Moving the primitive case to the top of this if chain and only having the assert case in the else branch would help readability a bit.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestAcmpEdgeCases.java line 201:

> 199:                 Float.intBitsToFloat(0xFFC00000), // negative quiet
> 200:                 Float.intBitsToFloat(0xFF800001), // negative signaling
> 201:                 Float.intBitsToFloat(0xFFFFFFFF),  // negative max payload

This looks like something that could be reused for many tests if it were in the testlibrary.

-------------

PR Review: https://git.openjdk.org/valhalla/pull/1823#pullrequestreview-3593105796
PR Review Comment: https://git.openjdk.org/valhalla/pull/1823#discussion_r2631359613
PR Review Comment: https://git.openjdk.org/valhalla/pull/1823#discussion_r2631201116
PR Review Comment: https://git.openjdk.org/valhalla/pull/1823#discussion_r2631230805
PR Review Comment: https://git.openjdk.org/valhalla/pull/1823#discussion_r2631263459
PR Review Comment: https://git.openjdk.org/valhalla/pull/1823#discussion_r2631323210