RFR: 8298952: All nodes should have type(n) == Value(n) after IGVN [v7]

Thu Feb 2 09:17:35 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 showed no significant runtime change.
> 
> **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? Update: probably would be overkill to notify for all those cases. I will leave the notification at the call-site, and improved the comment.
> - `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 pull request now contains 18 commits:

 - Fixed merge conflict with change by Tobias
 - Fixes for Christian's review suggestions
 - two small fixes and copyright 2023
 - Review suggestions by Tobias Hartmann v1

   Co-authored-by: Tobias Hartmann <tobias.hartmann at oracle.com>
 - Fix tests that directly query TypeProfileLevel, we had issues because I changed the type
 - 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
 - ... and 8 more: https://git.openjdk.org/jdk/compare/8d6e8a47...7bab82be

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

Changes: https://git.openjdk.org/jdk/pull/11775/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=11775&range=06
  Stats: 406 lines in 25 files changed: 284 ins; 35 del; 87 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