RFR: 8371536: C2: VerifyIterativeGVN should assert on first detected failure

[Benoît Maillard]

On Wed, 19 Nov 2025 09:30:45 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>>> The alternative would be to directly assert in the verify methods, but I suppose that would be a bigger code change.
>> 
>> Yes, I also considered it. I don't really have a strong opinion, but maybe you do. Asserting directly in the verify methods would allow us to have more targeted asserts, and more accurate reports for triaging. On the other side, as you mentioned, this would be more code changes.
>> 
>>> Hmm, I did see some cases in the verify methods that are maybe not directly "missed optimization opportunity" but some other kind of issue. Maybe we should assert directly for those, rather than returning and ending up at this assert.
>> 
>> These are not labelled as such in the printing, but I would argue these are still missed optimization opportunities, aren't they? I mean if things are still moving when calling `Ideal`, it means that this could have been done earlier.
>
> Honestly, I would do the changes, and just assert in the specific methods. That also helps us with more precise stack traces. The required changes are not that large and surely not that complicated, and we may actually end up with less code over all.
> 
> `Ideal optimization did not make progress but created new unused nodes.`
> 
> This one could get triggered even if we don't make progress at all. It may be that some `Ideal` optimization always generates nodes but then does not actually insert them into the graph. That could be considered wasteful, and that is really all that assert tells us. What do you think?

I agree, doing the changes sounds quite reasonable and is definitely worth it if it gives us more information.

> This one could get triggered even if we don't make progress at all. It may be that some Ideal optimization always generates nodes but then does not actually insert them into the graph. That could be considered wasteful, and that is really all that assert tells us. What do you think?

Mmh, I never saw that assert actually getting triggered, interesting. I agree as well, in this case it's a bit different. I will do the changes, thanks for the suggestions!

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

PR Review Comment: https://git.openjdk.org/jdk/pull/28295#discussion_r2541824373