On Thu, 18 Dec 2025 12:16:48 GMT, Tobias Hartmann <[email protected]> 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
