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

[Emanuel Peter]

On Tue, 10 Jun 2025 10:00:12 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> 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
>
> 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?

@chhagedorn While `assert(false)` would be correct, I think the check is a bit more expressive, that is why I left it in. But I guess the comment also says the same. Let me know what you prefer, I'm undecided.

> 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.

Having the whole conversation in a single JBS issue sounds a bit tricky... it is more like 100 different issues each with their own conversation.

And I don't yet know which nodes have to be fixed together, and which nodes have multiple problems.

I would also prefer if the comments were in the code - it's not that bad to create a JBS issue and commit the comments. That way, everything is in the code, and not spread over multiple JBS issues and GitHub conversations.

My suggestion is this:
- Use the umbrella issue: [JDK-8359103](https://bugs.openjdk.org/browse/JDK-8359103)
C2 VerifyIterativeGVN: Umbrella for extending Ideal and Identity verification (JDK-8347273)
- There, we can do some basic triaging, and then file subtasks.
- In the end, everything interesting to know needs to be committed back. Including text and pictures (ASCII).

@chhagedorn Would that work for you?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22970#discussion_r2137676530
PR Review Comment: https://git.openjdk.org/jdk/pull/22970#discussion_r2137674037