RFR: 8351414: C2: MergeStores must happen after RangeCheck smearing

[Quan Anh Mai]

On Fri, 7 Mar 2025 16:13:01 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> I would imagine `PhaseMergeStores` that looks at all stores in the graph, does the analysis and the transformation, then we can run another round of IGVN after that. I think a global view would be easier and more efficient than the local view from `StoreNode::Ideal`. Since `StoreNode::Ideal` needs to ensure that there is no store after it and just bail out otherwise.
>
> @merykitty I'm not sure if I understood you right. You seem to say that I should do this (please correct me):
> - Traverse the WHOLE graph, looking for stores (I would like to avoid that, that's why I carry around the list).
> - Rewrite the logic in MergeStores, and somehow have a global view rather than the local one. That's lots of work and I'm not sure I want to invest the time, unless it is somehow clearly better.
> 
> This is really a fix for Valhalla, see https://bugs.openjdk.org/browse/JDK-8348959, so it would be nice to fix this rather sooner. If someone wants to then refactor the code later, that's fine with me ;)

@eme64

> Traverse the WHOLE graph, looking for stores (I would like to avoid that, that's why I carry around the list).

I think keeping a list is fine.

> Rewrite the logic in MergeStores, and somehow have a global view rather than the local one. That's lots of work and I'm not sure I want to invest the time, unless it is somehow clearly better.

I think the logic would still be the same. We start at a store then try to find the last store in the chain, then group the stores and do the merge. After that, we can remove the replaced stores from the work list. It is global in the sense that we can freely walk the graph instead of being restricted to the current node that is invoking `Ideal`. This leads to a series of:

    Status status_use = find_adjacent_use_store(_store);
    if (status_use.found_store() != nullptr) {
      return nullptr;
    }

while we can do

    while (next != nullptr) {
      StoreNode* last = next;
      next = find_adjacent_use_store(last);
    }

It is also better because idealisation of a store node may be invoked several times, leading to useless `find_adjacent_use_store` invocations.

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

PR Comment: https://git.openjdk.org/jdk/pull/23944#issuecomment-2706891522