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

[Christian Hagedorn]

On Fri, 2 May 2025 07:54:31 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:
> 
>   added the test to JTREG

A few suggestions, otherwise, it looks good to me, too!

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

> 2458: //  Returns true if a failure occurred.
> 2459: bool PhaseMacroExpand::expand_macro_nodes() {
> 2460:   // Perform refining of strip mined loop node before we start to expand.

Maybe you can put the new code into a separate method.

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

> 2460:   // Perform refining of strip mined loop node before we start to expand.
> 2461:   for (int i = C->macro_count(); i > 0; i--) {
> 2462:     Node* n = C->macro_node(i-1);

Suggestion:

    Node* n = C->macro_node(i - 1);

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

> 2461:   for (int i = C->macro_count(); i > 0; i--) {
> 2462:     Node* n = C->macro_node(i-1);
> 2463:     if (n->Opcode() == Op_OuterStripMinedLoop) {

You could also use `is_OuterStripMinedLoop()`:
Suggestion:

    if (n->is_OuterStripMinedLoop()) {

test/hotspot/jtreg/compiler/macronodes/TestLoopStripMiningInMacroElimination.java line 48:

> 46:         }
> 47:     }
> 48: 

Suggestion:

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24890#pullrequestreview-2811688174
PR Review Comment: https://git.openjdk.org/jdk/pull/24890#discussion_r2071426238
PR Review Comment: https://git.openjdk.org/jdk/pull/24890#discussion_r2071423368
PR Review Comment: https://git.openjdk.org/jdk/pull/24890#discussion_r2071424370
PR Review Comment: https://git.openjdk.org/jdk/pull/24890#discussion_r2071424668