RFR: 8292660: C2: blocks made unreachable by NeverBranch-to-Goto conversion are removed incorrectly

[Vladimir Kozlov]

On Tue, 23 Aug 2022 08:26:57 GMT, Roberto Castañeda Lozano <rcastanedalo at openjdk.org> wrote:

> This changeset addresses three issues in the current removal of unreachable blocks after NeverBranch-to-goto conversion (introduced recently by [JDK-8292285](https://bugs.openjdk.org/browse/JDK-8292285)):
> 
> 1. The [unreachable block removal and pre-order index update loop](https://github.com/openjdk/jdk/blob/7b5f9edb59ef763acca80724ca37f3624d720d06/src/hotspot/share/opto/block.cpp#L613-L621) skips the block next to the removed one, and iterates beyond the end of the block list (`PhaseCFG::_blocks`). Skipping blocks can lead to duplicate pre-order indices (`Block::_pre_order`) and/or pre-order indices greater than the size of the block list, causing problems in later transformations.
> 
> 2. The [outer block traversal loop](https://github.com/openjdk/jdk/blob/7b5f9edb59ef763acca80724ca37f3624d720d06/src/hotspot/share/opto/block.cpp#L698-L729) iterates beyond the end of the block list whenever one or more unreachable blocks are removed.
> 
> 3. Transitively unreachable blocks (such as B10 in the following example), arising in methods with multiple infinite loops, are not removed:
> 
> ![transitive](https://user-images.githubusercontent.com/8792647/186109043-416213b7-8735-41de-9910-acf0997db095.png)
> 
> This changeset addresses issue 1 by decrementing the block count (`_number_of_blocks`) and block index (`i`) right after a block is removed from the block list, and issues 2 and 3 by decoupling NeverBranch-to-goto conversion from removal of unreachable code. Instead of removing the blocks eagerly, the removal is postponed to a second phase that works in an iterative worklist fashion, making it possible to remove transitively unreachable blocks such as B10 in the above example. For sanity, the changeset also ensures that reverse post-order indices (`Block::_rpo`) stay within the block list bounds, even though this does not seem to be assumed by any later transformation.
> 
> #### Testing
> 
> - tier1-3 (windows-x64, linux-x64, linux-aarch64, and macosx-x64; release and debug mode).
> - tier4-5 (linux-x64; debug mode).
> - tier1-3 (linux-x64; debug mode) using `-XX:-BlockLayoutByFrequency` (which interleaves NeverBranch-to-goto conversion with moving uncommon blocks to the end of the block list).
> - fuzzing (~1 h. on each platform).

src/hotspot/share/opto/block.cpp line 743:

> 741:         _blocks.remove(i);
> 742:         _number_of_blocks--;
> 743:         i--; // Ensure that we visit the block following the removed one.

May be `continue;` here to skip following checks for `dead` block.

src/hotspot/share/opto/block.cpp line 753:

> 751:         block->_rpo--;
> 752:       }
> 753:     }

Are you sure it will work for complex case? May be recalculate RPO and order after you removed dead blocks from graph.

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

PR: https://git.openjdk.org/jdk/pull/9976