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

[Roberto Castañeda Lozano]

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

> I will just run (...) a set of benchmarks to increase the confidence that this is indeed a very corner case.

I ran DaCapo 23 and did not hit the problematic case once. The regular case (exactly same type) is exercised by more than half of the DaCapo 23 benchmarks.

Will come back with test results in a day or two.

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

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