[lworld] RFR: 8372113: [lworld] Fix various issues with TypeAryPtr and TypeAryKlassPtr [v6]

[Tobias Hartmann]

On Mon, 8 Dec 2025 12:17:08 GMT, Quan Anh Mai <qamai at openjdk.org> wrote:

>> src/hotspot/share/opto/compile.cpp line 1418:
>> 
>>> 1416:       // Bottom array (meet of int[] and byte[] for example), accesses to it will be done with
>>> 1417:       // Unsafe. This should alias with all arrays. For now just leave it as it is (this is
>>> 1418:       // incorrect!).
>> 
>> Is there a tracking bug for this?
>
> Yes it is this one https://bugs.openjdk.org/browse/JDK-8331133

Okay, maybe we should reference that here or at least file a Valhalla specific issue to keep track of it.

>> src/hotspot/share/opto/compile.cpp line 1998:
>> 
>>> 1996:   if (_inline_type_nodes.length() == 0) {
>>> 1997:     // keep the graph canonical
>>> 1998:     igvn.optimize();
>> 
>> Why is this needed?
>
> It is because the other return path canonicalizes the graph, so it makes more sense for this path to do so, too. It is also that analyzing the memory graph in `adjust_flat_array_access_aliases` requires a canonical graph, or else we may encounter an `AddP` that has its type not yet computed.

Makes sense, thanks!

>> src/hotspot/share/opto/escape.cpp line 4630:
>> 
>>> 4628:         auto process_narrow_proj = [&](NarrowMemProjNode* proj) {
>>> 4629:           const TypePtr* adr_type = proj->adr_type();
>>> 4630:           const TypePtr* new_adr_type = tinst->with_offset(adr_type->offset());
>> 
>> Should this use `TypeAryPtr::add_field_offset_and_offset`?
>> 
>> Similar code also has a comment
>> 
>>       // In the case of a flat inline type array, each field has its
>>       // own slice so we need to extract the field being accessed from
>>       // the address computation
>
> It is equivalent, but since this is about mirroring the offset part from the old slice (without instance id) to the new slice, I think it is more trivial what is happening using `with_offset`.

Right, I hope we now caught all the places where this adjustment is needed.

>> src/hotspot/share/opto/macro.cpp line 548:
>> 
>>> 546:           }
>>> 547:           if (!init_value->is_InlineType()) {
>>> 548:             return nullptr;
>> 
>> How can this happen?
>
> `LibraryCallKit::inline_newArray` does not type check `init_val`, so if it is untyped (for example, the return value of `Class::new_instance`), then it will not be an `InlineTypeNode`.

Should we cast it to `InlineTypeNode` in `LibraryCallKit::inline_newArray` instead?

>> src/hotspot/share/opto/type.cpp line 679:
>> 
>>> 677:   TypeAryPtr::LONGS   = TypeAryPtr::make(TypePtr::BotPTR, TypeAry::make(TypeLong::LONG     ,TypeInt::POS, false, false, true, true, true), ciTypeArrayKlass::make(T_LONG),   true,  Offset::bottom);
>>> 678:   TypeAryPtr::FLOATS  = TypeAryPtr::make(TypePtr::BotPTR, TypeAry::make(Type::FLOAT        ,TypeInt::POS, false, false, true, true, true), ciTypeArrayKlass::make(T_FLOAT),  true,  Offset::bottom);
>>> 679:   TypeAryPtr::DOUBLES = TypeAryPtr::make(TypePtr::BotPTR, TypeAry::make(Type::DOUBLE       ,TypeInt::POS, false, false, true, true, true), ciTypeArrayKlass::make(T_DOUBLE), true,  Offset::bottom);
>> 
>> Could we use default arguments here?
>
> Yes, we can, but I feel that being explicit here would be better, as this combination will only make sense as the default for primitive arrays. What do you think?

That's fine with me. We can still change it later.

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/1755#discussion_r2610819202
PR Review Comment: https://git.openjdk.org/valhalla/pull/1755#discussion_r2610824732
PR Review Comment: https://git.openjdk.org/valhalla/pull/1755#discussion_r2610828692
PR Review Comment: https://git.openjdk.org/valhalla/pull/1755#discussion_r2610832249
PR Review Comment: https://git.openjdk.org/valhalla/pull/1755#discussion_r2610835377