RFR: 8339526: C2: store incorrectly removed for clone() transformed to series of loads/stores

[Christian Hagedorn]

On Thu, 2 Oct 2025 10:39:21 GMT, Roland Westrelin <roland at openjdk.org> wrote:

> In the `test1()` method of the test case:
> 
> `inlined2()` calls `clone()` for an object loaded from field `field`
> that has inexact type `A` at parse time. The intrinsic for `clone()`
> inserts an `Allocate` and an `ArrayCopy` nodes. When igvn runs, the
> load of `field` is optimized out because it reads back a newly
> allocated `B` written to `field` in the same method. `ArrayCopy` can
> now be optimized because the type of its `src` input is known. The
> type of its `dest` input is the `CheckCastPP` from the allocation of
> the cloned object created at parse time. That one has type `A`. A
> series of `Load`s/`Store`s are created to copy the fields of class `B`
> from `src` (of type `B`) to `dest` of (type `A`).
> 
> Writting to `dest` with offsets for fields that don't exist in `A`,
> causes this code in `Compile::flatten_alias_type()`:
> 
> 
>     } else if (offset < 0 || offset >= ik->layout_helper_size_in_bytes()) {
>       // Static fields are in the space above the normal instance
>       // fields in the java.lang.Class instance.
>       if (ik != ciEnv::current()->Class_klass()) {
>         to = nullptr;
>         tj = TypeOopPtr::BOTTOM;
>         offset = tj->offset();
>       }
> 
> 
> to assign it some slice that doesn't match the one that's used at the
> same offset in `B`.
> 
> That causes an assert in `ArrayCopyNode::try_clone_instance()` to
> fire. With a release build, execution proceeds. `test1()` also has a
> non escaping allocation. That one causes EA to run and
> `ConnectionGraph::split_unique_types()` to move the store to the non
> escaping allocation to a new slice. In the process, when it iterates
> over `MergeMem` nodes, it notices the stores added by
> `ArrayCopyNode::try_clone_instance()`, finds that some are not on the
> right slice, tries to move them to the correct slice (expecting they
> are from a non escaping EA). That causes some of the `Store`s to be
> disconnected. When the resulting code runs, execution fails as some
> fields are not copied.
> 
> The fix I propose is to skip `ArrayCopyNode::try_clone_instance()`
> when `src` and `dest` classes don't match as this seems like a rare
> enough corner case.

That looks reasonable to me.

src/hotspot/share/opto/arraycopynode.cpp line 220:

> 218:     // of the newly allocated cloned object (in dest). Exact type is now known (in src), but type for the cloned object
> 219:     // (dest) was not updated. When copying fields below, Store nodes may write to offsets for fields that don't exist in
> 220:     // the inexact class. The stores would then be assigned an incorrect slice.

Suggestion:

    // At parse time, the exact type of the object to clone was not known. That inexact type was captured by the CheckCastPP
    // of the newly allocated cloned object (in dest). The exact type is now known (in src), but the type for the cloned object
    // (dest) was not updated. When copying the fields below, Store nodes may write to offsets for fields that don't exist in
    // the inexact class. The stores would then be assigned an incorrect slice.

test/hotspot/jtreg/compiler/arraycopy/TestCloneUnknownClassAtParseTime.java line 47:

> 45:     }
> 46: 
> 47:     static A field;

Can you move the field up to the other field?

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/27604#pullrequestreview-3309935950
PR Review Comment: https://git.openjdk.org/jdk/pull/27604#discussion_r2410520602
PR Review Comment: https://git.openjdk.org/jdk/pull/27604#discussion_r2410498928