On Fri, 17 Apr 2026 05:24:32 GMT, Dean Long <[email protected]> wrote:
>> Christian Hagedorn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> improve assert message
>
> src/hotspot/share/opto/macroArrayCopy.cpp line 1504:
>
>> 1502: bool go_to_slow_path = src_elem != dest_elem || top_src->is_flat()
>> != top_dest->is_flat() || dest_elem == T_VOID;
>> 1503: if ((top_src->is_flat() && !top_src->elem()->is_inlinetypeptr()) ||
>> 1504: (top_dest->is_flat() && !top_dest->elem()->is_inlinetypeptr())) {
>
> Doesn't an `is_flat` array imply concrete elements here? I didn't think we
> could create a flat array with abstract elements. For example,
> ObjArrayKlass::array_layout_selection checks element->is_abstract().
> FlatArrayKlass asserts element_klass->is_inline_klass(). And
> FlatArrayKlass::element_klass() returns InlineKlass*.
At first sight, it looks like it must be implying it. It's true that we cannot
directly create a flat abstract value class array. But we could have this:
https://github.com/openjdk/valhalla/blob/8f06d5e939e11431a275ea0d5c171ffe9b3d67b1/test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestArrays.java#L3925-L3932
We create a flat array of a concrete value class in each branch and then merge
them together. The type system propagates the flatness information and we end
up with a flat abstract value class array. The elements are now abstract and
represented by an `InstanceKlass` and not an `InlineKlass` anymore. We don't
know the exact layout of the elements because it could either be `BadCastV1` or
`BadCastV2` or something else if there are even more sub classes of `BadCastA`
(the type system only sees `BadCastA`).
> src/hotspot/share/opto/macroArrayCopy.cpp line 1508:
>
>> 1506: } else if (top_dest->is_flat() &&
>> 1507: bs->array_copy_requires_gc_barriers(alloc != nullptr,
>> T_OBJECT, false, false, BarrierSetC2::Optimization) &&
>> 1508: top_dest->elem()->inline_klass()->contains_oops()) {
>
> && short-circuits and I see you swapped the order. We should do the cheaper
> operation first. Is that array_copy_requires_gc_barriers() or
> elem()->inline_klass()->contains_oops()?
I think `array_copy_requires_gc_barriers()` is cheaper since `contain_oops()`
also involves a guarded VM entry.
> src/hotspot/share/opto/macroArrayCopy.cpp line 1606:
>
>> 1604: Node* dest_length = (alloc != nullptr) ?
>> alloc->in(AllocateNode::ALength) : nullptr;
>> 1605: assert(top_src->is_flat() == top_dest->is_flat(), "must have bailed
>> out before");
>> 1606: if (top_src->is_flat() && same_nullness) {
>
> This is definitely simpler, but it takes me a couple seconds to see this is
> the opposite of `!top_src->is_flat() || !same_nullness`. Since we are
> guaranteed to need this expression at least once, how about doing something
> like:
>
> const bool same_nullness ...
> const bool flat_and_same_nullness = top_src->is_flat() && same_nullness;
>
> once, then we could have one `if (!flat_and_same_nullness)` and one `if
> (flat_and_same_nullness)`.
That's a good idea! Updated.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2325#discussion_r3098888838
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2325#discussion_r3098799450
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2325#discussion_r3098846652
