RFR: 8375038: C2: Enforce that Ideal() returns the root of the subgraph if any change was made by checking the node hash
Benoît Maillard
bmaillard at openjdk.org
Tue Jan 27 14:08:16 UTC 2026
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.
### Testing
- [x] [GitHub Actions](https://github.com/benoitmaillard/jdk/actions?query=branch%3AJDK-8375038)
- [x] tier1-3, plus some internal testing
- [x] Make sure we don't get any harness failure with the new flag bit for tier1
Thank you for reviewing!
-------------
Commit messages:
- Update copyright years
- Update copyright year to 2026 in cfgnode.cpp
- Update copyright year in c2_globals.hpp
- Update documentation for Node::Ideal
- Check hash before and after Ideal call and fix phinode
Changes: https://git.openjdk.org/jdk/pull/29421/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=29421&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8375038
Stats: 37 lines in 6 files changed: 23 ins; 0 del; 14 mod
Patch: https://git.openjdk.org/jdk/pull/29421.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/29421/head:pull/29421
PR: https://git.openjdk.org/jdk/pull/29421
More information about the hotspot-dev
mailing list