RFR: 8376220: C2: Refactor the logic to in MemNode::find_previous_store [v5]

[Roberto Castañeda Lozano]

On Sat, 24 Jan 2026 19:28:52 GMT, Quan Anh Mai <qamai at openjdk.org> wrote:

>> Hi,
>> 
>> This patch refactors the logic in `MemNode::find_previous_store` and makes a small improvement to `MemNode::detect_ptr_independence`. An IR test accompanies the improvement.
>> 
>> Please take a look and share your thoughts, thanks a lot.
>
> Quan Anh Mai has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add test store the loaded vector

Thanks for extracting this refactoring into an independent changeset! This is is going to simplify significantly the review process of the subsequent load folding changes. I have a few comments, questions, and suggestions.

src/hotspot/share/opto/memnode.cpp line 564:

> 562:     return true;
> 563:   }
> 564: 

Thanks for accompanying this changeset with some test cases! Could you add a few negative ones where the memory accesses cannot be folded (e.g. one where `c1` and `c2` in `TestFindStore.java` are of the exact same class, one when one is a subclass of the other, one that exercises the raw-to-oop casting you mention above, etc.)?

src/hotspot/share/opto/memnode.cpp line 709:

> 707:   } else if (adr_type->base() == TypePtr::AnyPtr) {
> 708:     // Give up on a very wide access
> 709:     return nullptr;

What kind of memory access is ruled out here? Could you add a test case for it? In mainline, this condition will imply `adr_maybe_raw` and impose an additional constraint on raw accesses (base equality), but not lead necessarily to `find_previous_store` giving up, right?

src/hotspot/share/opto/memnode.cpp line 740:

> 738:       }
> 739: 
> 740:       // If the bases are the same and the offsets are the same, it seems that this is the exact

Suggestion:

      // (b) If the bases are the same and the offsets are the same, it seems that this is the exact


In general, I find the original comments referring to steps (a), (b), (c), etc. useful and would prefer if they were left in besides return and continue statements below.

src/hotspot/share/opto/memnode.cpp line 741:

> 739: 
> 740:       // If the bases are the same and the offsets are the same, it seems that this is the exact
> 741:       // store we are looking for, the caller will check if the type of the store matches

Could you detail in the comment where does the caller check type matching?

src/hotspot/share/opto/memnode.cpp line 785:

> 783:       if (detect_ptr_independence(base, alloc, st_base, AllocateNode::Ideal_allocation(st_base), phase)) {
> 784:         // detect_ptr_independence == true means that it can prove that base and st_base cannot
> 785:         // have the same runtime value

I see how this comment can be useful in the original local EA changeset, but in the context of this separate changeset it seems redundant since it is basically restating what the comment two lines above says.

src/hotspot/share/opto/memnode.cpp line 1910:

> 1908:     ctrl = ctrl->in(0);
> 1909:     set_req(MemNode::Control,ctrl);
> 1910:     return this;

Is there a reason to return early in this changeset, or is it something that only makes sense in the context of the subsequent local EA changes? Same for the early return below and the IGVN recording at the end of the function.

test/hotspot/jtreg/compiler/c2/gvn/TestFindStore.java line 1:

> 1: /*

Thanks for adding these test cases!
Out of curiosity, I ran some testing disabling `MemNode::find_previous_store` entirely and found that we have very little "optimization check coverage" (tests with IR checks verifying that folding happens) for this logic -- only a couple of seemingly unrelated tests fail. It would be great if you could extend this test file with more positive and negative basic tests so that we have stronger confidence in 1) the correctness of this refactoring and the subsequent local EA changes and 2) that they do not accidentally inhibit some current optimization. Interesting cases are combinations of overlapping and non-overlapping, regular and mismatched memory accesses, array copies, etc. What do you think?

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

Changes requested by rcastanedalo (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/29390#pullrequestreview-3706631775
PR Review Comment: https://git.openjdk.org/jdk/pull/29390#discussion_r2728104271
PR Review Comment: https://git.openjdk.org/jdk/pull/29390#discussion_r2728118435
PR Review Comment: https://git.openjdk.org/jdk/pull/29390#discussion_r2728140161
PR Review Comment: https://git.openjdk.org/jdk/pull/29390#discussion_r2728121897
PR Review Comment: https://git.openjdk.org/jdk/pull/29390#discussion_r2728128693
PR Review Comment: https://git.openjdk.org/jdk/pull/29390#discussion_r2728159254
PR Review Comment: https://git.openjdk.org/jdk/pull/29390#discussion_r2728201738