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

[Emanuel Peter]

On Fri, 7 Mar 2025 15:47:33 GMT, Quan Anh Mai <qamai at openjdk.org> wrote:

> Do you think it would make sense to make a dedicated `PhaseMergeStores` instead?

Hmm, maybe? Can you say a little more how you imagine it?

@merykitty I basically just copied the structure from `post_loop_opts_phase`...

> 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 ;)

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

PR Comment: https://git.openjdk.org/jdk/pull/23944#issuecomment-2706816766
PR Comment: https://git.openjdk.org/jdk/pull/23944#issuecomment-2706829601
PR Comment: https://git.openjdk.org/jdk/pull/23944#issuecomment-2706858419