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

[Christian Hagedorn]

On Tue, 10 Jun 2025 11:36:17 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> 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.
>
> It would allow us to extend it further with a most significant bit of `E`, so that the order is `EDCBA`. If I do it in alphabetical order, then I would have to rename them. What do you think?

Okay, I only looked at it from a user-perspective that you might mismatch the description to the value passed to the flag. What could help here is reversing the order you mention the modes: first D:, then C: etc.

>> 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-8359103
>> )), 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?

I'm honestly not sure what the best way is. Currently, it feels a bit too verbose when also mentioning reproducers with command line options and failing tests which sounds more like things to keep track of in JBS. But I also see your point that having everything in the comments is quite handy and keeps everything in one part. Maybe we can find some middle ground when you move the "how to reproduce" to the umbrella JBS? The rest you can keep in the comments, I'm fine with that and see its benefit.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22970#discussion_r2137814182
PR Review Comment: https://git.openjdk.org/jdk/pull/22970#discussion_r2137840332