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... That looks good to me. Thanks for fixing! > To move forward with this patch and enable more testing again, I suggest to > fix this second issue separately, either with > [JDK-8358079](https://bugs.openjdk.org/browse/JDK-8358079) or a new issue. Let's adjust JDK-8358079 to track this. ------------- Marked as reviewed by thartmann (Committer). PR Review: https://git.openjdk.org/valhalla/pull/2370#pullrequestreview-4188118272
