RFR: 8347273: C2: VerifyIterativeGVN for Ideal and Identity

[Manuel Hässig]

On Wed, 8 Jan 2025 14:43:40 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.
> 
> Testing passed tier1-3, with extra timeout factor 20.

Thank you for working on this and diligently noting the exceptions. That is quite the todo list you discovered 😅. It is great to see more verification going in.

I noted some minor things, mostly in the comments. With or without those, this looks good to me.

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

> 683:           "            B: verify that type(n) == n->Value() after IGVN"     \
> 684:           "            C: verify all Node::Ideal were applied in IGVN"      \
> 685:           "            D: verify all Node::Identity were applied in IGVN"   \

Suggestion:

          "            C: verify Node::Ideal did not miss opportunities"    \
          "            D: verify Node::Identity did not miss opportunities" \

For me, "all `Node::Ideal` were applied" parsed weirdly, so I tried my hand at an alternative formulation. Feel free to ignore.

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

> 1186: }
> 1187: 
> 1188: // Check that all Ideal optimizations that could be done were done.

Suggestion:

// Check that all Ideal optimizations that could be done were done.
// Returns true if it found missed optimization opportunities and false otherwise and for exceptions.

The return value was not immediately clear to me.

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

> 1801:   }
> 1802:   tty->print_cr("The result after Ideal:");
> 1803:   i->dump_bfs(1, nullptr, "");

Perhaps taking the tty lock might be appropriate, due to the amount of printing? Or do we know that nothing else is printing?

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

> 1805: }
> 1806: 
> 1807: // Check that all Identity optimizations that could be done were done.

Suggestion:

// Check that all Identity optimizations that could be done were done.
// Returns true if it found missed optimization opportunities and false otherwise and for exceptions.

As above.

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

> 1946: 
> 1947:   if (n->is_Load()) {
> 1948:     // LoadNode::Identity tries to look for an earier store value via

Suggestion:

    // LoadNode::Identity tries to look for an earlier store value via

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

> 1989:   n->dump_bfs(1, nullptr, "");
> 1990:   tty->print_cr("New node:");
> 1991:   i->dump_bfs(1, nullptr, "");

Suggestion:

  // The verificatin just found a new Identity that was not found during IGVN.
  tty->cr();
  tty->print_cr("Missed Identity optimization:");
  tty->print_cr("Old node:");
  n->dump_bfs(1, nullptr, "");
  tty->print_cr("New node:");
  i->dump_bfs(1, nullptr, "");

The wording of the comment confused me a bit 😅 

Also, perhaps taking the tty lock might be appropriate since you are printing a lot here? Or do we know that only verification is printing at this point?

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

Marked as reviewed by mhaessig (Author).

PR Review: https://git.openjdk.org/jdk/pull/22970#pullrequestreview-2912495849
PR Review Comment: https://git.openjdk.org/jdk/pull/22970#discussion_r2137284714
PR Review Comment: https://git.openjdk.org/jdk/pull/22970#discussion_r2137242097
PR Review Comment: https://git.openjdk.org/jdk/pull/22970#discussion_r2137266075
PR Review Comment: https://git.openjdk.org/jdk/pull/22970#discussion_r2137243146
PR Review Comment: https://git.openjdk.org/jdk/pull/22970#discussion_r2137259432
PR Review Comment: https://git.openjdk.org/jdk/pull/22970#discussion_r2137257629