RFR: 8298952: All nodes should have type(n) == Value(n) after IGVN [v2]
Emanuel Peter
epeter at openjdk.org
Mon Jan 30 10:40:58 UTC 2023
> I want to verify that **type(n) == Value(n)** after every IGVN round. Basically: all optimizations that can be made should be made.
>
> FYI: this is a part of a bigger effort to verify IGVN, and refactor the notification code for CCP and IGVN:
> [JDK-8298951](https://bugs.openjdk.org/browse/JDK-8298951) `Umbrella: improve CCP and IGVN verification`
> I recently implemented CCP Node::Value verification [JDK-8257197](https://bugs.openjdk.org/browse/JDK-8257197).
>
> I'm extending the flag `-XX:+VerifyIterativeGVN` (used to enable Use-Def verification) to `-XX:VerifyIterativeGVN=XY`, where `X` can enable the Value verification (low overhead), and `Y` enables the Use-Def verification (very slow, requires increasing timeouts).
>
> This patch has passed tier1-5 and stress testing. Performance testing is running...
>
> **I would love to hear your opinion, especially about the special cases, but also my fixes named below.**
>
> For the verification to pass, I had to **fix some things**:
> - Add `gvn.transform` to some `new Node` cases where it was missing, else I got `type(n) == nullptr`.
> - Add missing notification logic in `PhaseIterGVN::add_users_to_worklist`: notify tripcount phi of LongCountedLoop if the limit changes (int case was already implemented), notify Sub node if input CastII has changing input, notify And in shift-and (shift-mask) operation. Every optimization should have corresponding notification if (recursive) inputs change. These were easy to fix examples.
> - `ConstraintCastNode::Value` violated the idempotence guarantee: subsequent calls to Value returned different values. Fixed that.
> - `CastLLNode::Ideal` can be widened after loop-opts, with the goal of commoning CastLL nodes. For that, we need to call `record_for_post_loop_opts_igvn` for them, even before IGVN (when `can_reshape` is still false). They may otherwise not land on the post loop-opts worklist.
> - During CCP, some integer types are saturated (widened). But IGVN could narrow them again. So add them to IGVN worklist if they are saturated (widened) during CCP.
> - `kill_dead_code`: tries to remove dead nodes recursively, in analogy to what IGVN would do, but more efficiently. Unfortunately, we do not call `add_users_to_worklist` for the dead nodes, which IGVN would do. I do the notification once per dead node now. `kill_dead_code` could be refactored and made more efficient, maybe we can do that in a future RFE.
> - `PhaseIdealLoop::clone_loop_handle_data_uses` calls `_igvn.replace_input_of(use, idx, phi)`, but does not notify the users. I got a failure in tier5-common, where use was a CmpI of a loop-exit test, and the tripcount-phi needed to be notified. I added notification here, but I wonder if it needs to be added to `replace_input_of` generally?
> - `CastII`: implemented notification for `CmpI -> Bool -> If -> IfProj -> CastII`. This handles case where previously we have `in1 != X`, see comment in code.
> - `Sub` with input of chain of `ConstraintCast`: rare case, implemented a BFS traversal down the casts, to the `Sub` nodes.
> - Replaced a `set_req` with `set_req_X` in `CmpPNode::Ideal`, as it removed the last use of the previous input, and lead to `fatal error: no reachable node should have no use`.
>
> As with CCP Node::Value verification, a few cases have to be **exempt from verification (special cased)**.
> https://github.com/openjdk/jdk/blob/fe5552be65e9dd9c044d41e8295bb176d9754e28/src/hotspot/share/opto/phaseX.cpp#L1244-L1296
>
> - `Integer` widen: expected, must special case.
> - `Load`: must special case because of deep traversal in Value. We may be able to make special case logic more precise. One might also experiment with deep notification, but that could be expensive and complex. I would leave special case as is, and address improvements in a future RFE.
> - `CmpP`: deep traversal of CFG to determine domination (independence between two pointers). Probably needs to be special cased.
>
> **Note**: I want to address these `memory` optimizations in a follow up RFE. We had the idea of simply putting all these nodes on a special list that is always processed during IGVN. We will have to investigate performance impact either way.
Emanuel Peter has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 13 additional commits since the last revision:
- Merge branch 'master' into JDK-8298952
- Update after review from Vladimir K
- Merge branch 'master' into JDK-8298952
- fix guard issue
- Merge branch 'master' into JDK-8298952
- make VerifyIterativeGVN fine grained: can enable Def-Use and Value verification separately
- replace set_req with set_req_X after: fatal error: no reachable node should have no use
- implement Cast-chain traversal for Sub
- sanity assert from last commit was wrong - turned it into actual logic
- add verification to VerifyIterativeGVN, and implement CmpI / If / CastII pattern notification
- ... and 3 more: https://git.openjdk.org/jdk/compare/7f6ad9c8...41af5e82
-------------
Changes:
- all: https://git.openjdk.org/jdk/pull/11775/files
- new: https://git.openjdk.org/jdk/pull/11775/files/6ce2768c..41af5e82
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=11775&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=11775&range=00-01
Stats: 9750 lines in 467 files changed: 2225 ins; 758 del; 6767 mod
Patch: https://git.openjdk.org/jdk/pull/11775.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/11775/head:pull/11775
PR: https://git.openjdk.org/jdk/pull/11775
More information about the hotspot-compiler-dev
mailing list