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

[Roberto Castañeda Lozano]

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).

Thanks for the review, Vladimir! I have updated the changeset to postpone removal of unreachable blocks to after `PhaseCFG::fixup_flow()`. This makes the handling of block indices simpler and less error-prone: at this stage `Block::_pre_order` does not contain pre-order indices but simple block list indices, and `Block::_rpo` does not contain valid reverse post-order indices anymore (they are invalidated by block insertions in `PhaseCFG::insert_goto_at()`). I have also added a `continue` as suggested.

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

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