[lworld] RFR: 8372113: [lworld] Fix various issues with TypeAryPtr and TypeAryKlassPtr [v6]
Tobias Hartmann
thartmann at openjdk.org
Mon Dec 8 11:33:33 UTC 2025
On Sat, 6 Dec 2025 17:12:36 GMT, Quan Anh Mai <qamai at openjdk.org> wrote:
>> Hi,
>>
>> This PR fixes various issues with `TypeAryPtr` and `TypeAryKlassPtr`. It starts with me trying to tighten the properties (`flat`, `null_free`, etc) of these classes, then fixing all the revealed issues until there are no crashes or wrong results left.
>>
>> Please take a look and leave your reviews, thanks a lot.
>
> Quan Anh Mai has updated the pull request incrementally with one additional commit since the last revision:
>
> Should not clone MergeMem
All tests pass now - great work! Thanks for this major effort in cleaning up the logic around arrays. This is tricky to review but I did a pass and added some comments.
src/hotspot/share/ci/ciObjArrayKlass.cpp line 201:
> 199: ciInstanceKlass* ik = base->as_instance_klass();
> 200: // Even though MyValue is final, [LMyValue is only exact if the array
> 201: // is null-free due to null-free [LMyValue <: null-able [LMyValue.
There are some more occurrences of `// Even though MyValue is final, [LMyValue is only exact if the array` in the code, should those be updated as well?
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?
src/hotspot/share/opto/compile.cpp line 1485:
> 1483: cast_to_ptr_type(TypePtr::BotPTR)->
> 1484: cast_to_exactness(false)->
> 1485: with_offset(offset);
These changes are difficult to review because it's not immediately clear what changed semantically. For example, where did this line go?
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?
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
src/hotspot/share/opto/macro.cpp line 548:
> 546: }
> 547: if (!init_value->is_InlineType()) {
> 548: return nullptr;
How can this happen?
src/hotspot/share/opto/memnode.cpp line 1355:
> 1353: Node* init_value = ld_alloc->in(AllocateNode::InitValue);
> 1354: if (init_value != nullptr) {
> 1355: // TODO 8350865 Scalar replacement does not work well for flat arrays.
Can you re-enable the tests in `TestArrays.java` marked with `// TODO 8350865 Scalar replacement does not work well for flat arrays` now? There are a few more places in the code marked with `// TODO 8350865 Scalar replacement ...` as well. We might just want to leave this to [JDK-8350865](https://bugs.openjdk.org/browse/JDK-8350865) though.
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?
src/hotspot/share/opto/type.cpp line 6686:
> 6684: k = ciObjArrayKlass::make(k->element_klass(), true);
> 6685: } else {
> 6686: k = ciObjArrayKlass::make(k->element_klass(), false);
You can just use `k = ciObjArrayKlass::make(k->element_klass(), refined);` here (couldn't directly suggest that change because there's an unmodified line in-between).
src/hotspot/share/prims/jvm.cpp line 493:
> 491: } else {
> 492: props = ArrayKlass::ArrayProperties::NULL_RESTRICTED;
> 493: }
Suggestion:
ArrayKlass::ArrayProperties props = ArrayKlass::ArrayProperties::NULL_RESTRICTED;
if (vk->has_non_atomic_layout()) {
props |= ArrayKlass::ArrayProperties::NON_ATOMIC;
}
Maybe double-check, if my suggestion even compiles :slightly_smiling_face:
Regarding:
> When creating a refined ObjArrayKlass, we fall back to ref array if the corresponding flat layout is not present, which means that if the element type is not a LooselyConsistentValue, ValueClass::newNullRestrictedNonAtomicArray will make a ref array. This seems counter-intuitive, it should have fallen back to a null-restricted atomic array instead.
I tend to agree but this is still debatable. @fparain Thoughts?
-------------
PR Review: https://git.openjdk.org/valhalla/pull/1755#pullrequestreview-3551098324
PR Review Comment: https://git.openjdk.org/valhalla/pull/1755#discussion_r2598238554
PR Review Comment: https://git.openjdk.org/valhalla/pull/1755#discussion_r2597949776
PR Review Comment: https://git.openjdk.org/valhalla/pull/1755#discussion_r2597955310
PR Review Comment: https://git.openjdk.org/valhalla/pull/1755#discussion_r2597897503
PR Review Comment: https://git.openjdk.org/valhalla/pull/1755#discussion_r2597971076
PR Review Comment: https://git.openjdk.org/valhalla/pull/1755#discussion_r2597911813
PR Review Comment: https://git.openjdk.org/valhalla/pull/1755#discussion_r2597996905
PR Review Comment: https://git.openjdk.org/valhalla/pull/1755#discussion_r2597919984
PR Review Comment: https://git.openjdk.org/valhalla/pull/1755#discussion_r2598022672
PR Review Comment: https://git.openjdk.org/valhalla/pull/1755#discussion_r2597849855
More information about the valhalla-dev
mailing list