[lworld] RFR: 8357474: [lworld] Consolidate load/store flat inline types [v8]

[Tobias Hartmann]

On Fri, 23 May 2025 06:33:48 GMT, Quan Anh Mai <qamai at openjdk.org> wrote:

>> Hi,
>> 
>> This patch consolidates loading and storing of flat inline types. This makes it so that `make_from_flat` and `store_flat` will take a pointer to the target location. `make_from_flat_array` and `store_flat_array` are split out into their own methods which receive an `idx` parameter. This also fixes some issues I theorycrafted:
>> 
>> - Improper loading/storing of atomic elements in a non-atomic container. This is not supported at the moment but it would be great to prepare for it.
>> - `is_naturally_atomic` checks `nof_declared_nonstatic_fields`, which is incorrect if the sole field has multiple fields. I refactored it into `ciInlineKlass`, the decision to do atomic loads/stores is also delegated to the callee, the caller only needs to know whether the operations act as if they are atomic, not whether they are actually executed atomically.
>> - `null_free` of an oop can only be trusted if the container itself is null-free. For example this case:
>> 
>>         value class StringHolder {
>>             @NullRestricted
>>             String s;
>>         }
>> 
>>         value class StringHolderHolder {
>>             // flatten
>>             StringHolder v;
>>         }
>> 
>> Then `v.s` cannot be trusted to be non-null because `v` can be null.
>> 
>> This also allows straightforward implementation of https://bugs.openjdk.org/browse/JDK-8349110.
>> 
>> Please let me know what you think. Thanks very much.
>
> Quan Anh Mai has updated the pull request incrementally with one additional commit since the last revision:
> 
>   weird layout

Changes requested by thartmann (Committer).

src/hotspot/share/opto/graphKit.cpp line 1880:

> 1878:   arytype = arytype->cast_to_exactness(true);
> 1879:   arytype = arytype->cast_to_not_null_free(is_not_null_free);
> 1880:   array = _gvn.transform(new CheckCastPPNode(control(), array, arytype, ConstraintCastNode::StrongDependency));

Did you hit an issue without this?

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

> 486:       if (base->is_Con() && oop_ptr->is_inlinetypeptr() && !is_array && !mismatched) {
> 487:         // If the oop to the inline type is constant (static final field), we can
> 488:         // also treat the fields as constants because the inline type is immutable.

Why is this not needed anymore?

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

> 714:   kit->C->set_flat_accesses();
> 715:   ciInlineKlass* vk = inline_klass();
> 716:   assert(vk->maybe_flat_in_array(), "");

Please add an assert message

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

> 720:     region = new RegionNode(3);
> 721:     region->init_req(1, kit->top());
> 722:     region->init_req(2, kit->top());

Why is the top init needed? Aren't `nullptr` inputs handled the same way in `RegionNode::Ideal` etc.?

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

> 722:     region->init_req(2, kit->top());
> 723:   } else {
> 724:     region = new RegionNode(4);

For simplicity, can't we always have a `RegionNode` with 4 inputs here and simply leave `in(3)` as top/nullptr if we don't need it?

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

> 752:     Node* ptr = kit->flat_array_element_address(cast, idx, vk, /* null_free */ false, /* not_null_free */ true, /* atomic */ true);
> 753:     if (vk->get_field_by_offset(vk->payload_offset(), false) == nullptr && vk->null_marker_offset_in_payload() != 0) {
> 754:       // TODO 8357612 Weird layout

I don't understand why this would be specific to the layout described in [JDK-8357612](https://bugs.openjdk.org/browse/JDK-8357612)? Wouldn't we also hit `vk->get_field_by_offset(vk->payload_offset(), false) == nullptr` when the null marker is at `vk->payload_offset()` which would be totally fine?

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

> 766:   }
> 767: 
> 768:   kit->set_control(kit->IfTrue(iff_null_free));

Maybe add a `// Null-free` comment here.

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

> 1385:         assert(vk->has_non_atomic_layout(), "");
> 1386:         // TODO 8350865 Is the conversion to/from payload folded? We should wire this directly.
> 1387:         // Also remove the PreserveReexecuteState in Parse::array_load when buffering is no longer possible.

Now that you removed the `convert_to_payload` hack, this comment should be removed, right? Also, buffering should no longer be possible and the referenced code in `Parse::array_load` can be removed.

src/hotspot/share/opto/parse2.cpp line 131:

> 129:           arytype = arytype->cast_to_exactness(true);
> 130:           arytype = arytype->cast_to_not_null_free(is_not_null_free);
> 131:           Node* flat_array = gvn().transform(new CastPPNode(control(), array, arytype));

I assume you are not using `flat_array_element_address` anymore because you don't need the address but only want to cast the array but I think this casting should then be factored out. Also, why don't you need a `ConstraintCastNode::StrongDependency` here?

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

PR Review: https://git.openjdk.org/valhalla/pull/1470#pullrequestreview-2867770479
PR Review Comment: https://git.openjdk.org/valhalla/pull/1470#discussion_r2106895201
PR Review Comment: https://git.openjdk.org/valhalla/pull/1470#discussion_r2106898542
PR Review Comment: https://git.openjdk.org/valhalla/pull/1470#discussion_r2106900806
PR Review Comment: https://git.openjdk.org/valhalla/pull/1470#discussion_r2106910662
PR Review Comment: https://git.openjdk.org/valhalla/pull/1470#discussion_r2106916828
PR Review Comment: https://git.openjdk.org/valhalla/pull/1470#discussion_r2107279838
PR Review Comment: https://git.openjdk.org/valhalla/pull/1470#discussion_r2107272385
PR Review Comment: https://git.openjdk.org/valhalla/pull/1470#discussion_r2107044110
PR Review Comment: https://git.openjdk.org/valhalla/pull/1470#discussion_r2107016830