RFR: 8347273: C2: VerifyIterativeGVN for Ideal and Identity [v3]

[Christian Hagedorn]

On Tue, 10 Jun 2025 09:32:46 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 two additional commits since the last revision:
> 
>  - Update src/hotspot/share/opto/phaseX.cpp
>    
>    Co-authored-by: Manuel Hässig <manuel at haessig.org>
>  - review suggestions, and handled a few more edge cases

Great effort and analysis! I have some comments/questions.

src/hotspot/share/opto/c2_globals.hpp line 686:

> 684:           "            C: verify Node::Ideal did not miss opportunities"    \
> 685:           "            D: verify Node::Identity did not miss opportunities" \
> 686:           "A, B, C, and D in 0=off; 1=on")                                  \

Why don't you use ABCD? It seems strange/unexpected to reverse the alphabetical order.

src/hotspot/share/opto/phaseX.cpp line 1090:

> 1088:       if (is_verify_Ideal())    { failure |= verify_node_Ideal(n, false); }
> 1089:       if (is_verify_Ideal())    { failure |= verify_node_Ideal(n, true); }
> 1090:       if (is_verify_Identity()) { failure |= verify_node_Identity(n); }

Suggestion: How about naming them `verify_Value/Ideal/Identity_for(n)`?

src/hotspot/share/opto/phaseX.cpp line 1126:

> 1124:     node->dump();
> 1125:   }
> 1126:   assert(_worklist.size() == 0, "igvn worklist must still be empty after verify");

The `_worklist` size does not seem to change after the bailout on L1114. So, we know that here the worklist is non-empty. Would `assert(false)` fit better?

src/hotspot/share/opto/phaseX.cpp line 1196:

> 1194: // Returns true if it found missed optimization opportunities and
> 1195: //         false otherwise (no missed optimization, or skipped verification).
> 1196: bool PhaseIterGVN::verify_node_Ideal(Node* n, bool can_reshape) {

General comment about your analysis for Ideal and Identity for why you disabled some of the verification. Very thorough and nicely explained! I'm wondering though if we should just open a tracking JBS issue (we could use JDK-8347273), dump the analysis there and refer to that JBS issue from the code for further details. This would allow us to use some permalinks from GitHub (we should probably not post them in the code directly) or extend the analysis with additional images etc. You also included a lot of best guesses (which is totally understandable!) which we might want to extend, comment further on in a discussion, or update because we know more about them. For that, we would need to update the actual code each time which seems unfortunate - and we might not fix some things because it does not seem worth the effort for tiny mistakes or updates. In JBS this comes for free.

What do you think about that? Of course, in the end, it's also a trade-off.

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

PR Review: https://git.openjdk.org/jdk/pull/22970#pullrequestreview-2912749936
PR Review Comment: https://git.openjdk.org/jdk/pull/22970#discussion_r2137403056
PR Review Comment: https://git.openjdk.org/jdk/pull/22970#discussion_r2137472659
PR Review Comment: https://git.openjdk.org/jdk/pull/22970#discussion_r2137441404
PR Review Comment: https://git.openjdk.org/jdk/pull/22970#discussion_r2137434095