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

[kuaiwei]

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 I'm working on merging loads and I meet the same problem. I work around it by delay transform LoadNode after all range check smearing. But I think your solution is better. Could you change the name "_for_merge_stores_igvn" as "_for_merge_mem_igvn" and it can be used both for store and loads?

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

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