RFR: 8341976: C2: use_mem_state != load->find_exact_control(load->in(0)) assert failure [v3]

[Christian Hagedorn]

On Fri, 14 Mar 2025 09:59:59 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> The `arraycopy` writes to a non escaping array so its `ArrayCopy` node
>> is marked as having a narrow memory effect. One of the loads from the
>> destination after the copy is transformed into a load from the source
>> array (the rationale being that if there's no load from the
>> destination of the copy, the `arraycopy` is not needed). The load from
>> the source has the input memory state of the `ArrayCopy` as memory
>> input. That load is then sunk out of the loop and its control is
>> updated to be after the `ArrayCopy`. That's legal because the
>> `ArrayCopy` only has a narrow memory effect and can't modify the
>> source. The `ArrayCopy` can't be eliminated and is expanded. In the
>> process, a `MemBar` that has a wide memory effect is added. The load
>> from the source has control after the membar but memory state before
>> and because the membar has a wide memory effect, the load is anti
>> dependent on the membar: the graph is broken (the load can't be pinned
>> after the membar and anti dependent on it).
>> 
>> In short, the problem is that the graph is transformed under the
>> assumption that the `ArrayCopy` has a narrow effect but the
>> `ArrayCopy` is expanded to a subgraph that has a wide memory
>> effect. The fix I propose is to not insert a membar with a wide memory
>> effect. We still need a membar when the destination is non escaping
>> because the expanded `ArrayCopy`, if it writes to a tighly allocated
>> array, writes to raw memory and not to the destination memory slice.
>
> 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 nine additional commits since the last revision:
> 
>  - more
>  -  Merge branch 'master' into JDK-8341976
>  - more
>  - exp
>  - fix
>  - Merge branch 'master' into HEAD
>  - review
>  - whitespace
>  - fix & test

That looks reasonable. I've launched some testing and results look good so far (there is quite some load at the moment - will take a bit longer to complete than usual).

src/hotspot/share/opto/macroArrayCopy.cpp line 826:

> 824:   }
> 825: 
> 826:   if (is_partial_array_copy) {

Why is this check no longer required?

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23465#pullrequestreview-2701568242
PR Review Comment: https://git.openjdk.org/jdk/pull/23465#discussion_r2004987093