RFR: 8375038: C2: Enforce that Ideal() returns the root of the subgraph if any change was made by checking the node hash

[Roberto Castañeda Lozano]

On Mon, 26 Jan 2026 14:47:58 GMT, Benoît Maillard <bmaillard at openjdk.org> wrote:

> This PR introduces an assert in `PhaseIterGVN` to check that `Ideal` actually returns something if the node was modified.
> 
> ## Context
> 
> In the description of `Node::Ideal` in `node.cpp`, we have:
> 
>> If ANY change is made, it must return the root of the reshaped graph - even if the root is the same Node
> 
> It is crucial that such changes do not go unnoticed and that they can propagate to other nodes. Current documentation also states:
> 
>> Running with `-XX:VerifyIterativeGVN=1` checks
>> these invariants, although its too slow to have on by default.  If you are
>> hacking an Ideal call, be sure to test with `-XX:VerifyIterativeGVN=1`
> 
> However, `-XX:VerifyIterativeGVN=1` ends up veryfing that the `_in` and `_out` arrays are consistent, but does not verify the return value. 
> 
> This PR aims to enforce the return value invariant. It should also make regression testing of bugs caused by wrongly returning nullptr in `Ideal` easier, such as [JDK-8373251](https://bugs.openjdk.org/browse/JDK-8373251).
> 
> ## Proposed Change
> 
> In summary, this PR brings the following set of changes
> - Add a new flag bit to`-XX:VerifyIterativeGVN` for verifying return of `Ideal` calls
> - Add an assert on the hash of nodes before and after `Ideal` in `PhaseIterGVN::transform_old`
> - Fix `Ideal` optimizations that would cause harness errors with testing on tier1
> - Update the comments in the code to clarify the invariant and how to enforce it
> 
> After consideration, I took the decision to only check the hash if the node is not dead. It seems there are many cases where the control node is dead, and we propagate the information to all users with `kill_dead_code`, and end up return `nullptr`. This is basically a mechanism to "speed up" the propagation (it would also happen normally via the usual IGVN worklist). This somehow contradicts the "must return the root of the reshaped graph" invariant, but it seems to be a common practice.
> 
> In addition to that, I have decided to implement this as part of a new flag bit to `-XX:VerifyIterativeGVN` instead of an existing one, because there is a risk that it causes new failures in existing usages of the flag.
> 
> This PR is meant to introduce the new check and fix the most "obvious" failures that the new flag would introduce in common scenarios, such as when running with `-version` on tier1. Since there are known issues caused by bad return values of `Ideal` (such as [JDK-8373251](https://bugs.openjdk.org/browse/JDK-8373251)), I will fix other failures in follow-up PRs....

src/hotspot/share/opto/node.cpp line 1157:

> 1155: // can help with validating these invariants, although they are too slow to have on by default:
> 1156: //    - '-XX:VerifyIterativeGVN=1' checks the def-use info
> 1157: //    - '-XX:VerifyIterativeGVN=100000' cheks the return value

Suggestion:

//    - '-XX:VerifyIterativeGVN=100000' checks the return value

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

PR Review Comment: https://git.openjdk.org/jdk/pull/29421#discussion_r2732251370