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

[Quan Anh Mai]

On Mon, 26 Jan 2026 16:03:16 GMT, Roberto Castañeda Lozano <rcastanedalo at openjdk.org> wrote:

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

@robcasloz Thanks for the reviews, I have addressed them.

@dlunde That's a good idea, I have compared the old and the new implementation of `MemNode::find_previous_store`. I tried verifying that either the old implementation returns `null`, returns the same value as the new one, or the refactored implementation returns `top`. I ran tier1-tier7 and encountered no violation.

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

I didn't encounter any, but theoretically, the only case possible for a `MemNode` to have this kind of `adr_type` is if the base turns out to be a `null`. I think it is necessary to exclude this case early, because such an address will upset `Compile::get_alias_index`. In mainline I think it is just too rare or impossible so we do not encounter a crash.

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

I have added them back

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

Done

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

Done, I have removed them

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

I think it is more readable. The variable `progress` is defined at the start, set only here and the following early return, then used all the way at the end of the function. It is better for comprehension to return right after making changes to the node.

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

PR Comment: https://git.openjdk.org/jdk/pull/29390#issuecomment-3828740258
PR Review Comment: https://git.openjdk.org/jdk/pull/29390#discussion_r2749686450
PR Review Comment: https://git.openjdk.org/jdk/pull/29390#discussion_r2749686855
PR Review Comment: https://git.openjdk.org/jdk/pull/29390#discussion_r2749686555
PR Review Comment: https://git.openjdk.org/jdk/pull/29390#discussion_r2749686726
PR Review Comment: https://git.openjdk.org/jdk/pull/29390#discussion_r2749688624