On Mon, 27 Apr 2026 11:38:32 GMT, Christian Hagedorn <[email protected]> wrote:
> ### Failing Test > The assert added in mainline to verify that `adr_type` and `adr`'s type match > fails in Valhalla when facing a load from a cloned flat array (i.e. called > `clone()`) in `TestArrays::test28()`: > https://github.com/openjdk/valhalla/blob/cfda30f3735276318c291718ca1f3daaa0e124c8/test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestArrays.java#L808-L813 > > This triggers when trying to add array elements to safepoints. > > ### Mismatched `AddP` Type vs. Calculated `adr_type` > In `PhaseMacroExpand::make_arraycopy_load()`, we have the following type for > the `base` of the flat array `src`: > > // gvn.type(base) with _offset = 0, _field_offset = bottom: > aryptr:flat:instptr:.../MyValue2 (...):NotNull:exact *,iid=bot[int:10] > (...):NotNull:exact:flat(+bot):null free > > We then try to add the flat array load for `src.x` which is at `offset = 32` > (16 bytes for the array object header + another 16 bytes to get to `x` at > offset 16 (`src.v` is first at offset 0)). Here we get a mismatch between the > computed `adr_type` and the type stored for `adr` (i.e. an `AddP`) which is > later used when creating the `LoadNode`: > > // adr_type with _offset = 32, _field_offset = bottom: > aryptr:flat:instptr:.../MyValue2 (...):NotNull:exact *,iid=bot[int:10] > (...):NotNull:exact:flat(+bot):null free[2] > > // gvn.type(adr) with _offset = 16, _field_offset = 16: > aryptr:flat:instptr:.../MyValue2 (...):NotNull:exact *,iid=bot[int:10] > (...):NotNull:exact:flat(+16):null free[0] > > The types have different `_field_offset` values and thus end up on a > different alias class which triggers the assert. > > ### Reason for Mismatch > The problem is that in `PhaseMacroExpand::make_arraycopy_load()`, we simply > call `add_offset()`, regardless of whether it's a flat array or not: > https://github.com/openjdk/valhalla/blob/cfda30f3735276318c291718ca1f3daaa0e124c8/src/hotspot/share/opto/macro.cpp#L323-L326 > > This ignores `_field_offset` and leaves it at `bottom`. But when calling > `AddPNode::Value()`, we call `TypeAryPtr::add_field_offset_and_offset()` > which also considers `_field_offset` and properly sets it to 16 and > correspondingly `_offset` to 16. > > ### Fixing the Mismatch > A straight forward fix is to use `add_field_offset_and_offset()` instead of > `add_offset()` when having an array pointer. This works to address the > observing assertion failure. This is what's suggested in this patch. I also > applied some small clean-ups. > > ### Another Lurking Bug > In `add_field_offset_and_offset()`, we have special code to find out what t... src/hotspot/share/opto/type.cpp line 5786: > 5784: > 5785: const TypePtr* TypeAryPtr::add_field_offset_and_offset(intptr_t offset) > const { > 5786: if (!is_flat() || !klass_is_exact() || offset == OffsetBot || offset > == OffsetTop) { Removed unneeded `Type::` prefix. src/hotspot/share/opto/type.cpp line 5788: > 5786: if (!is_flat() || !klass_is_exact() || offset == OffsetBot || offset > == OffsetTop) { > 5787: return add_offset(offset); > 5788: } I added a quick bailout here for non-flat and abstract value class arrays to have fewer nested levels below. src/hotspot/share/opto/type.cpp line 5792: > 5790: // Handle flat concrete value class array with known 'offset' which > could refer to an actual field in the flat storage. > 5791: int adj = 0; > 5792: if (_offset != Offset::bottom && _offset != Offset::top) { Simplified to use `operator!=()` and `operator==()` instead of calling `get()`. src/hotspot/share/opto/type.cpp line 5808: > 5806: int shift = flat_log_elem_size(); > 5807: int mask = (1 << shift) - 1; > 5808: int field_offset = static_cast<int>((offset - header) & mask); Added `static_cast` here (IDE warning about narrowing type). ------------- PR Review Comment: https://git.openjdk.org/valhalla/pull/2370#discussion_r3146991141 PR Review Comment: https://git.openjdk.org/valhalla/pull/2370#discussion_r3146977972 PR Review Comment: https://git.openjdk.org/valhalla/pull/2370#discussion_r3146991995 PR Review Comment: https://git.openjdk.org/valhalla/pull/2370#discussion_r3146969506
