RFR: 8347273: C2: VerifyIterativeGVN for Ideal and Identity [v7]
Christian Hagedorn
chagedorn at openjdk.org
Thu Jun 12 06:53:39 UTC 2025
On Tue, 10 Jun 2025 14:44:48 GMT, Emanuel Peter <epeter at openjdk.org> wrote:
>> **Past Work**
>> With https://github.com/openjdk/jdk/pull/11775 / [JDK-8298952](https://bugs.openjdk.org/browse/JDK-8298952) we added `Node::Value` verification.
>>
>> **This PR**
>> I'm now adding verification for `Ideal` and `Identity`. I'm adding two bits to the flag `VerifyIterativeGVN`.
>>
>> I found many many node types that hit my verification assert, i.e. that could still be optimized after IGVN is over, just because these nodes were not put on the worklist any more.
>>
>> My approach was to aggressively bail-out for all nodes that had an issue. This way, we can address one by one in follow-up RFEs. For many, I did some initial assessment, and left some comments about what issues I encountered.
>>
>> **Future Work:**
>> In many cases, the issue is just a missing notification when inputs of inputs are changed. These would be good starter tasks. But there are probably also more complicated cases. And there are surely cases where verification will be impossible, because it is possible that the Idea / Identity optimizations traverse longer paths, and we cannot expect that notification makes it down that path. For those cases, we will have to leave the exception and document it well.
>>
>> I filed:
>> [JDK-8359103](https://bugs.openjdk.org/browse/JDK-8359103) C2 VerifyIterativeGVN: Umbrella for extending Ideal and Identity verification (JDK-8347273)
>> (We can file subtasks for the nodes we want to fix. I don't want to file them all now, but we should file them as we are investigating, so that there is no duplicate work.)
>>
>> Testing passed tier1-3, with extra timeout factor 20.
>
> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
>
> reorder flags for Christian
Some more minor comments but otherwise looks good, thanks for the updates and for the offline discussion to share some background!
> @chhagedorn I checked with @TobiHartmann : he said he does not have a strong opinion, but if he had to make a decision, he would prefers having everything in the comments.
Then let's go with everything in the comments - might be better in the end since we eventually want to replace the individual comments with final verdicts or have the cases fixed anyway at some point :-)
src/hotspot/share/opto/phaseX.cpp line 1204:
> 1202: switch (n->Opcode()) {
> 1203: // RangeCheckNode::Ideal looks up the chain for about 999 nodes
> 1204: // see "Range-Check scan limit". So it is possible that something
Suggestion:
// (see "Range-Check scan limit"). So, it is possible that something
src/hotspot/share/opto/phaseX.cpp line 1205:
> 1203: // RangeCheckNode::Ideal looks up the chain for about 999 nodes
> 1204: // see "Range-Check scan limit". So it is possible that something
> 1205: // optimized in that input subgraph, and the RangeCheck was not
Suggestion:
// is optimized in that input subgraph, and the RangeCheck was not
src/hotspot/share/opto/phaseX.cpp line 1256:
> 1254: // "Useless" means that there is no code in either branch of the If.
> 1255: // I found a case where this was not done yet during IGVN.
> 1256: // Why does the Region not get added to IGVN worklist when the If diamond becomes useles?
Suggestion:
// Why does the Region not get added to IGVN worklist when the If diamond becomes useless?
src/hotspot/share/opto/phaseX.cpp line 1316:
> 1314: case Op_AddD:
> 1315: //case Op_AddI: // Also affected for other reasons.
> 1316: //case Op_AddL: // Also affected for other reasons.
Suggestion:
//case Op_AddI: // Also affected for other reasons, see case further down.
//case Op_AddL: // Also affected for other reasons, see case further down.
src/hotspot/share/opto/phaseX.cpp line 1432:
> 1430: // x + (0 - [8424 AddL])
> 1431: // but the AddL was not added to the IGVN worklist. Investigate why.
> 1432: // There could be other issues too. For example with "commute", see above.
Suggestion:
// There could be other issues, too. For example with "commute", see above.
src/hotspot/share/opto/phaseX.cpp line 1444:
> 1442: // This has the effect that these new nodes end up on the IGVN worklist,
> 1443: // but if we now leave verification and IGVN itself, we have nodes on the
> 1444: // worklist, and that should not be (there are asserts against this).
Sounds like we just need some exception when calling `Ideal` on a `SubTypeCheck` that we can have certain nodes still on the worklist like a `cmp`. Maybe we should add this as a suggestion to the comment?
Suggestion:
// but if we now leave verification and IGVN itself, we have nodes other
// than 'n' still on the worklist. This will fail with an assert in
// verify_empty_worklist(). Maybe we just need to add an exception and
// check that only certain nodes like 'cmp' are still on the worklist. After
// this check, we can clear the worklist such that verify_empty_worklist()
// succeeds.
src/hotspot/share/opto/phaseX.cpp line 1685:
> 1683: // Found in tier1-3.
> 1684: case Op_CMoveI:
> 1685: return false;
Maybe merge them together and add a comment that you have not investigated further (I assume?) since you found them all in tier1-3 without more specific details.
-------------
Marked as reviewed by chagedorn (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/22970#pullrequestreview-2913983773
PR Review Comment: https://git.openjdk.org/jdk/pull/22970#discussion_r2138142398
PR Review Comment: https://git.openjdk.org/jdk/pull/22970#discussion_r2138143758
PR Review Comment: https://git.openjdk.org/jdk/pull/22970#discussion_r2138146868
PR Review Comment: https://git.openjdk.org/jdk/pull/22970#discussion_r2138150068
PR Review Comment: https://git.openjdk.org/jdk/pull/22970#discussion_r2141804882
PR Review Comment: https://git.openjdk.org/jdk/pull/22970#discussion_r2141810963
PR Review Comment: https://git.openjdk.org/jdk/pull/22970#discussion_r2141830386
More information about the hotspot-dev
mailing list