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

[Christian Hagedorn]

On Thu, 18 Dec 2025 12:16:48 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 with a new target base due to a merge or a rebase. The pull request now contains 21 commits:
> 
>  - Merge branch 'lworld' into JDK-8228361
>  - More refactoring, better comments
>  - More comments, removing workaround
>  - Merge
>  - v5
>  - Refactoring v4
>  - Refactoring v3
>  - Refactoring, more checks, new test
>  - Merge branch 'lworld' into JDK-8228362
>  - All tests pass
>  - ... and 11 more: https://git.openjdk.org/valhalla/compare/3c41c2aa...4f8556c7

Nice work! Some small comments, otherwise, it looks good to me.

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

> 1187:     Node* left = in(TypeFunc::Parms);
> 1188:     Node* right = in(TypeFunc::Parms + 1);
> 1189:     if (!left->is_top() && !right->is_top() && (left->is_InlineType() || right->is_InlineType())) {

I think you can remove the `is_top()` checks since the `is_InlineType()` checks already cover that.

Suggestion:

    if (left->is_InlineType() || right->is_InlineType()) {

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

> 671: 
> 672: // Check if a substitutability check between 'this' and 'other' can be implemented in IR
> 673: bool InlineTypeNode::can_emit_substitutability_check(Node* other) {

Can be made `const`. Same for `check_substitutability()`.

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

> 710: 
> 711:     Node* field_base = base;
> 712:     Node* field_ptr = ptr;

To better follow the logic below, I suggest the following names:
- `val` -> `this_field`
- `ptr`/`field_ptr` -> `other`/`field_other`. 

And further down use this/other instead of left/right.

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

> 720:       assert(base != nullptr, "lost base");
> 721:       field_ptr = igvn->register_new_node_with_optimizer(new AddPNode(base, ptr, igvn->MakeConX(field_off)));
> 722:       if (!field_is_flat(i)) {

How about adding a `InlineTypeNode::field()` method and then directly calling `is_flat()`, `is_null_free()` on the field instead of reloading it each time over the index?

src/hotspot/share/opto/library_call.cpp line 4424:

> 4422: }
> 4423: 
> 4424: //---------------------------load_mirror_from_klass----------------------------

Suggestion:

src/hotspot/share/opto/subnode.cpp line 932:

> 930:   //   CmpL(OrL(CastP2X(..), CastP2X(..)), 0L)
> 931:   // that are used by acmp to implement a "both operands are null" check.
> 932:   // See also the corresponding code in CmpPNode::Ideal.

I suggest to move it above as a method comment.

src/hotspot/share/opto/subnode.cpp line 1219:

> 1217:     // If one operand is an inline type, convert this to a "both operands are null" check:
> 1218:     //   CmpL(OrL(CastP2X(..), CastP2X(..)), 0L)
> 1219:     // CmpLNode::Ideal might optimize this further to avoid keeping buffer allocations alive.

Why is this possible?

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

Marked as reviewed by chagedorn (Committer).

PR Review: https://git.openjdk.org/valhalla/pull/1823#pullrequestreview-3591965308
PR Review Comment: https://git.openjdk.org/valhalla/pull/1823#discussion_r2630422825
PR Review Comment: https://git.openjdk.org/valhalla/pull/1823#discussion_r2630435789
PR Review Comment: https://git.openjdk.org/valhalla/pull/1823#discussion_r2630806019
PR Review Comment: https://git.openjdk.org/valhalla/pull/1823#discussion_r2630474288
PR Review Comment: https://git.openjdk.org/valhalla/pull/1823#discussion_r2630818564
PR Review Comment: https://git.openjdk.org/valhalla/pull/1823#discussion_r2630845361
PR Review Comment: https://git.openjdk.org/valhalla/pull/1823#discussion_r2630855979