On Mon, 8 Dec 2025 12:17:08 GMT, Quan Anh Mai <[email protected]> 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
