RFR: 8359121: C2: Region added by vectorizedMismatch intrinsic can survive as a dead node after IGVN

[Christian Hagedorn]

On Wed, 11 Jun 2025 11:35:28 GMT, Marc Chevalier <mchevalier at openjdk.org> wrote:

> In `bool LibraryCallKit::inline_vectorizedMismatch()` the region created at:
> 
> https://github.com/openjdk/jdk/blob/0582bd290d5a8b6344ae7ada36492cc2f33df050/src/hotspot/share/opto/library_call.cpp#L6502
> 
> may have only one input, and be a copy (that is no self-loop) and a single input. It is thus safe to remove. Yet, in the reproducer case, the node is short-circuited, but stays in the graph after IGVN.
> 
> Left, after Parsing/before IGVN; right, after IGVN:
> <img src="https://github.com/user-attachments/assets/2fc0e817-ccf5-4586-81dc-ea9daef21274" width="400">  <img src="https://github.com/user-attachments/assets/6032f54e-75ba-49a1-b00e-ac8053d9f592" width="400">
> On the left, the ⚠ is there because the Region doesn't have a self loop, which is expected for copies. On the right, it still doesn't have a self-loop, but IGV is also complaining the Region has no successor.
> 
> This transformation comes from `IfNode::Ideal`, that calls `IfNode::Ideal_common`, that calls `Node::remove_dead_region`,  that shortcuts a trivial Region input:
> https://github.com/openjdk/jdk/blob/0582bd290d5a8b6344ae7ada36492cc2f33df050/src/hotspot/share/opto/node.cpp#L1480-L1484
> 
> Yet, the Region node is never enqueued for IGVN, so stays in the graph.  This is not nice because it gives both:
> - a control node (50 Proj) has 2 successors,
> - a control node (46 Region) has 0 successors.
> 
> While at the end of IGVN, we could expect the graph to be cleaned up. There are couple ways of doing, each being enough by itself:
> 1. explicitly record for IGVN the region node in `LibraryCallKit::inline_vectorizedMismatch`, and not hoping it would be collected by another consequence
> 2. change `Node::remove_dead_region` to use `set_req_X` instead of `set_req`, so that if the Region goes dead, it will be removed.
> 3. not introduce the region node in `LibraryCallKit::inline_vectorizedMismatch` if we are going to have only one path, and thus avoid the problem entirely.
> 
> The solution 3. is really not easy and would require quite some code restructuring for simply saving removing a node. After discussing with @chhagedorn, we concluded that the solution 1. was probably the best:
> - usually, functions similar to `inline_vectorizedMismatch` call `record_for_igvn` themselves,
> - unlike what I assumed at first seeing `remove_dead_region`, it's not a general problem at all: I couldn't find another case without using `inline_vectorizedMismatch` where the Region is put aside, and not entirely disconnected quickly after, but the Regi...

Nice analysis and summary! As you've already mentioned in the description, we found solution 1 to be the best fit.

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25749#pullrequestreview-2917191481