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

[Emanuel Peter]

On Mon, 13 Oct 2025 15:28:01 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.
>
> Roland Westrelin has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
> 
>  - review
>  - Merge branch 'master' into JDK-8339526
>  - review
>  - Merge branch 'master' into JDK-8339526
>  - Update src/hotspot/share/opto/arraycopynode.cpp
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>  - test & fix

> 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 Loads/Stores are created to copy the fields of class B
from src (of type B) to dest of (type A).

I'm still struggling to understand. I wonder if the test can be further simplified to make the case more clear.

Am I understanding right, that we essentially this:

field = new B(42, 42, 42);
A a = field;
return (A)a.clone();

What should the result of that be? An `A` or a `B`? I think we should be getting a `B`, right? So why is the `dest` of the `ArrayCopy` an `A`? Is that even correct?

> 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.

How do you know that this is a rare case? Did you do some kind of profiling / benchmarking?

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

PR Comment: https://git.openjdk.org/jdk/pull/27604#issuecomment-3398083365