RFR: 8347515: C2: assert(!success || (C->macro_count() == (old_macro_count - 1))) failed: elimination must have deleted one node from macro list [v4]

[Christian Hagedorn]

On Wed, 7 May 2025 11:21:45 GMT, Saranya Natarajan <duke at openjdk.org> wrote:

>> Issue: The assertion failure , `assert(!success || (C->macro_count() == (old_macro_count - 1))) failed: elimination must have deleted one node from macro list`, occurs when [loop striping mining  ](https://bugs.openjdk.org/browse/JDK-8186027)may create a [MaxL](https://bugs.openjdk.org/browse/JDK-8324655)  after macro expansion.
>> 
>> Analysis : Before the macro nodes are expanded in` expand_macro_nodes`, there is a process where nodes from the macro list are eliminated. This also includes elimination of  any `OuterStripMinedLoop` node in the macro list. The bug occurs due to the refining of the strip mined loop in `adjust_strip_mined_loop` function just before it is eliminated. In this case, a` MaxL` node is added to the macro list in `adjust_strip_mined_loop`. 
>> 
>> Fix: The fix involves performing the refining of the strip mined loop before elimination process. More specifically, moving the `adjust_strip_mined_loop` function outside the elimination loop.
>> 
>> Improvement:  The process of eliminating macro nodes by calling `eliminate_macro_nodes` and performing additional Opaque and LoopLimit nodes elimination in ` expand_macro_nodes` is unintuitive as suggested in [JDK-8325478 ](https://bugs.openjdk.org/browse/JDK-8325478) and the current fix should be moved along with the other elimination code.
>
> Saranya Natarajan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   addressing review comments

Two more minor things but then it's good to go from my side :-)

src/hotspot/share/opto/macro.cpp line 2353:

> 2351: }
> 2352: 
> 2353: //------------------refine_strip_mined_loop_macro_node-------------------

Those headers were used in legacy code and you still see quite some of them around. But nowadays, we usually remove them when touching these methods or at least we don't add them for new methods.

Suggestion:

src/hotspot/share/opto/macro.cpp line 2471:

> 2469: //  Returns true if a failure occurred.
> 2470: bool PhaseMacroExpand::expand_macro_nodes() {
> 2471:   // Perform refining of strip mined loop before expanding macro nodes.

You can probably now remove this comment since the method name is expressive enough.

Suggestion:

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24890#pullrequestreview-2824220024
PR Review Comment: https://git.openjdk.org/jdk/pull/24890#discussion_r2079148056
PR Review Comment: https://git.openjdk.org/jdk/pull/24890#discussion_r2079149958