[lworld] RFR: 8335256: [lworld] C2: Remove larval InlineTypeNode [v5]
Tobias Hartmann
thartmann at openjdk.org
Fri May 9 10:33:16 UTC 2025
On Tue, 6 May 2025 00:13:24 GMT, Quan Anh Mai <qamai at openjdk.org> wrote:
>> Hi
>>
>> The root issue is that a larval value object is fundamentally different from a non-larval one. The most important thing is that it has a unique identity and it expects any modification on 1 reference observable by all other equivalent references. As a result, we need a mechanism to track the identity of a larval object, which `InlineTypeNode` does not provide. My current proposal to fix this issue is to abandon larval `InlineTypeNode`s and use the oop like other objects.
>>
>> It is probably beneficial to have another mechanism to make it easier optimizing larval inline type nodes, but I think it can be a follow-up RFE.
>>
>> An example regarding the issue with tracking the identity of a larval InlineTypeNode:
>>
>> Consider this pseudobytecode sequence:
>>
>> new MyValue;
>> dup;
>> loop;
>> invokespecial MyValue::<init>;
>> areturn;
>>
>> There are 2 equivalent references in the stack at the loop entry. When `Parse::merge` encounters them, it cannot know that these are the same because the back-edge has not been processed yet. As a result, it creates 2 separate `Phi`s for these references. Then, `invokespecial` will only make the top of the stack a non-larval object, not the next one, which is the one returned to the caller. As a result, we fail with `assert(!value->is_InlineType() || !value->as_InlineType()->is_larval(), "returning a larval")`. Worse, if the method is osr-compiled at the loop head, we have 2 separate references fed into the compiled function and there is no way we may know that they are of the same object.
>>
>> Please take a look and leave your reviews, thanks a lot.
>
> Quan Anh Mai has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains six commits:
>
> - move progress check
> - Merge branch 'lworld' into larvaloop
> - fast path for non intrinsics
> - fix test failures
> - Merge branch 'lworld' into larvaloop
> - remove larval InlineTypeNode
Thanks a lot for this great work and your patience, @merykitty. All tests passed and I had a detailed look at the changes. Looks all good to me, I just left a few questions / comments / suggestions.
src/hotspot/share/opto/compile.cpp line 2847:
> 2845:
> 2846: {
> 2847: // Eliminate some macro nodes before EA to reduce analysis pressure
Is this something we should consider doing in mainline?
src/hotspot/share/opto/compile.cpp line 2900:
> 2898: }
> 2899: igvn.set_delay_transform(false);
> 2900: print_method(PHASE_ITER_GVN_AFTER_ELIMINATION, 2);
Why did you remove the `igvn.optimize();` here?
src/hotspot/share/opto/doCall.cpp line 707:
> 705: }
> 706:
> 707: // Scalarize value objects passed into this invocation because we know that they are not larval
Suggestion:
// Scalarize value objects passed into this invocation if we know that they are not larval
src/hotspot/share/opto/doCall.cpp line 891:
> 889: if (cg->method()->is_object_constructor() && receiver != nullptr && gvn().type(receiver)->is_inlinetypeptr()) {
> 890: InlineTypeNode* non_larval = InlineTypeNode::make_from_oop(this, receiver, gvn().type(receiver)->inline_klass());
> 891: // Relinquish the oop input, we will delay the allocation to the point it is needed
It would be good to explain in more detail why we need to give up the oop input here.
src/hotspot/share/opto/graphKit.cpp line 1504:
> 1502: }
> 1503:
> 1504: Node* GraphKit::cast_non_larval(Node* obj) {
Suggestion:
Node* GraphKit::cast_to_non_larval(Node* obj) {
src/hotspot/share/opto/graphKit.cpp line 1510:
> 1508:
> 1509: const Type* obj_type = gvn().type(obj);
> 1510: if (!obj_type->is_inlinetypeptr()) {
I think this should be merged with the if above.
src/hotspot/share/opto/inlinetypenode.cpp line 1097:
> 1095: // Use base oop if fields are loaded from memory, don't do so if base is the CheckCastPP of an
> 1096: // allocation because the only case we load from a naked CheckCastPP is when we exit a
> 1097: // constructor of an inline type and we want to relinquish the larval oop there
As mentioned above, it would be good to explain why we need to relinquish the oop.
src/hotspot/share/opto/inlinetypenode.cpp line 1100:
> 1098: Node* base = is_loaded(phase);
> 1099: if (base != nullptr && !base->is_InlineType() && !phase->type(base)->maybe_null() && AllocateNode::Ideal_allocation(base) == nullptr) {
> 1100: if (oop != base || phase->type(is_buffered) != TypeInt::ONE) {
Maybe use `InlineTypeNode::is_allocated` here instead?
src/hotspot/share/opto/inlinetypenode.cpp line 1492:
> 1490: res->set_oop(kit->gvn(), alloc_oop);
> 1491: }
> 1492: // TODO 8239003
Should we close/update [JDK-8239003](https://bugs.openjdk.org/browse/JDK-8239003) now?
src/hotspot/share/opto/inlinetypenode.cpp line 1741:
> 1739: // Equivalent InlineTypeNodes are merged by GVN, so we just need to search for AllocateNode users to find redundant allocations.
> 1740: void InlineTypeNode::remove_redundant_allocations(PhaseIdealLoop* phase) {
> 1741: // TODO 8332886 Really needed? GVN is disabled anyway.
Should [JDK-8332886](https://bugs.openjdk.org/browse/JDK-8332886) be updated? Can the tests marked with `// TODO 8332886` in `TestBasicFunctionality.java` be enabled again?
src/hotspot/share/opto/macro.cpp line 2907:
> 2905: while (C->macro_count() > 0) {
> 2906: if (iteration++ > 100) {
> 2907: assert(false, "Too slow convergence of macro elimination");
Is this something that should be upstreamed to mainline?
src/hotspot/share/opto/macro.cpp line 2955:
> 2953: case Node::Class_Unlock:
> 2954: success = eliminate_locking_node(n->as_AbstractLock());
> 2955: #ifndef PRODUCT
Why is this change needed?
src/hotspot/share/opto/macro.cpp line 2986:
> 2984: }
> 2985:
> 2986: // If an allocation is used only in safepoints, elimination of another macro nodes can remove
Suggestion:
// If an allocation is used only in safepoints, elimination of another macro node can remove
src/hotspot/share/opto/parse1.cpp line 186:
> 184: bad_type_exit->control()->add_req(bad_type_ctrl);
> 185: }
> 186: l = gen_checkcast(l, makecon(tp->as_klass_type()->cast_to_exactness(true)), &bad_type_ctrl, false, true);
Please add a comment explaining why we need to pass `true` here.
src/hotspot/share/opto/parse1.cpp line 625:
> 623: const Type* t = _gvn.type(parm);
> 624: if (t->is_inlinetypeptr()) {
> 625: if (!is_osr_parse() && !(method()->is_object_constructor() && i == 0)) {
A comment would be good here
src/hotspot/share/opto/parse1.cpp line 1782:
> 1780: }
> 1781: if (t != nullptr && t != Type::BOTTOM) {
> 1782: if (!t->is_inlinetypeptr()) {
I think below code needs more comments, it's really hard to read.
src/hotspot/share/opto/parse1.cpp line 1799:
> 1797: } else {
> 1798: if (!n->is_InlineType()) {
> 1799: // We cannot blindly expand an inline type here since it may be larval
Suggestion:
// We cannot blindly scalarize an inline type oop here since it may be larval
src/hotspot/share/opto/parse1.cpp line 1801:
> 1799: // We cannot blindly expand an inline type here since it may be larval
> 1800: if (gvn().type(n)->is_zero_type()) {
> 1801: // Null constant implies that this is not a larval objects
Suggestion:
// Null constant implies that this is not a larval object
src/hotspot/share/opto/parse1.cpp line 1804:
> 1802: map()->set_req(j, InlineTypeNode::make_null(gvn(), t->inline_klass()));
> 1803: } else if (phi != nullptr && phi->is_InlineType()) {
> 1804: // Larval oops cannot be merged with non-larval ones
Suggestion:
// Larval oops cannot be merged with non-larval ones and since the merge point is non-larval, 'n' must be non-larval as well
src/hotspot/share/opto/parse2.cpp line 3600:
> 3598: IdealGraphPrinter* printer = C->igv_printer();
> 3599: char buffer[256];
> 3600: jio_snprintf(buffer, sizeof(buffer), "Bytecode %d: %s, map: %d", bci(), Bytecodes::name(bc()), map() == nullptr ? -1 : map()->_idx);
Looks like this should be fixed in mainline?
src/hotspot/share/opto/parse3.cpp line 260:
> 258: val = cast_non_larval(val);
> 259:
> 260: assert(!obj->is_InlineType(), "InlineTypeNodes are non-larval value objects");
The assert message is a bit confusing. Maybe improve it or add a comment that the field holder should not have been scalarized because it's a larval.
-------------
Marked as reviewed by thartmann (Committer).
PR Review: https://git.openjdk.org/valhalla/pull/1447#pullrequestreview-2827293741
PR Review Comment: https://git.openjdk.org/valhalla/pull/1447#discussion_r2081098023
PR Review Comment: https://git.openjdk.org/valhalla/pull/1447#discussion_r2081099460
PR Review Comment: https://git.openjdk.org/valhalla/pull/1447#discussion_r2081110476
PR Review Comment: https://git.openjdk.org/valhalla/pull/1447#discussion_r2081258219
PR Review Comment: https://git.openjdk.org/valhalla/pull/1447#discussion_r2081258646
PR Review Comment: https://git.openjdk.org/valhalla/pull/1447#discussion_r2081259816
PR Review Comment: https://git.openjdk.org/valhalla/pull/1447#discussion_r2081268646
PR Review Comment: https://git.openjdk.org/valhalla/pull/1447#discussion_r2081272123
PR Review Comment: https://git.openjdk.org/valhalla/pull/1447#discussion_r2081281915
PR Review Comment: https://git.openjdk.org/valhalla/pull/1447#discussion_r2081279707
PR Review Comment: https://git.openjdk.org/valhalla/pull/1447#discussion_r2081313953
PR Review Comment: https://git.openjdk.org/valhalla/pull/1447#discussion_r2081324229
PR Review Comment: https://git.openjdk.org/valhalla/pull/1447#discussion_r2081319481
PR Review Comment: https://git.openjdk.org/valhalla/pull/1447#discussion_r2081327425
PR Review Comment: https://git.openjdk.org/valhalla/pull/1447#discussion_r2081329157
PR Review Comment: https://git.openjdk.org/valhalla/pull/1447#discussion_r2081395622
PR Review Comment: https://git.openjdk.org/valhalla/pull/1447#discussion_r2081400005
PR Review Comment: https://git.openjdk.org/valhalla/pull/1447#discussion_r2081392727
PR Review Comment: https://git.openjdk.org/valhalla/pull/1447#discussion_r2081401220
PR Review Comment: https://git.openjdk.org/valhalla/pull/1447#discussion_r2081347200
PR Review Comment: https://git.openjdk.org/valhalla/pull/1447#discussion_r2081371648
More information about the valhalla-dev
mailing list