RFR: 8370200: Crash: assert(outer->outcnt() >= phis + 2 - be_loads && outer->outcnt() <= phis + 2 + stores + 1) failed: only phis

[Roland Westrelin]

On Mon, 8 Dec 2025 12:53:31 GMT, Daniel Lundén <dlunden at openjdk.org> wrote:

> Thanks for working on this @rwestrel!
> 
> > Now, PhiNode::Identity for 94 could replace it with the bottom memory phi with same inputs 451. But it doesn't run. It last ran between 3) and 4) and there's no reason for igvn to execute it again because 4) doesn't cause 94 to change in any way.
> 
> Just to double check, does `VerifyIterativeGVN` identify this missed transformation? If not, we should make sure it does.

It doesn't because `PhaseIterGVN::verify_Identity_for()` skips `Phi` nodes. All issues related to `Phi` nodes in `VerifyIterativeGVN` would need to be fixed first.

> > The fix I propose is to mirror the transformation from PhiNode::Identity in PhiNode::Ideal so the end result doesn't depend on what phi is modified and processed by igvn last.
> 
> Correct me if I'm wrong, but do we not achieve the same thing if we identify and add 94 to the worklist after the transformation of 93 -> 451? This possibly seems like a cleaner solution to me (see my code comment below).

In principle, yes.
The question is how do you reliably get 94 on the igvn queue.
In this particular case, `PhiNode::Ideal()` creates 451 and enqueues it on the igvn queue with `register_new_node_with_optimizer()`. Do we want to add custom logic in `PhiNode::Ideal()` to also enqueue all memory `Phi`s that are uses of the region?
It's likely not sufficient in the general case as, maybe, the transformation can only happen once one input of the bottom `Phi` is changed. So do we need something like `PhaseIterGVN::add_users_of_use_to_worklist()` as well? It wouldn't quite the same as we wouldn't enqueue uses of a use but the uses of a common input (the region)?
Or rather than having logic in a couple different places to enqueue the non bottom memory `Phi`, maybe, we can do that in `PhiNode::Ideal` for the bottom `Phi` which would essentially bethe patch I propose but, instead of making any change to the graph, it would enqueue the non bottom `Phi` so `PhiNode::Identity` does the change. It seems a bit wasteful to delay the change to the graph when it can be done safely in `PhiNode::Ideal` for the bottom memory `Phi` which is why I went with the change I propose.

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

PR Comment: https://git.openjdk.org/jdk/pull/28677#issuecomment-3631139093