RFR: 8325030: PhaseMacroExpand::value_from_mem_phi assert with "unknown node on this path" [v3]

[Christian Hagedorn]

On Wed, 15 Jan 2025 09:44:28 GMT, Theo Weidmann <tweidmann at openjdk.org> wrote:

>> The following code triggers an assert:
>> 
>> 
>> class Test {
>>     static class A {
>>         int a;
>>     }
>> 
>>     public static void main(String[] strArr) {
>>         int i2 = 0;
>>         for (int i = 0; i < 50; ++i)
>>             try {
>>                 synchronized (new A()) {
>>                     synchronized (Test.class) {
>>                         for (int var19 = 0; var19 < Integer.valueOf(i2);) {
>>                             Integer.valueOf(var19);
>>                         }
>>                     }
>>                 }
>>                 for (int var8 = 0; var8 < 10000; ++var8) ;
>>             } catch (ArithmeticException a_e) {
>>             }
>>     }
>> }
>> 
>> 
>> 
>> #  Internal Error (/Users/theo/jdk/open/src/hotspot/share/opto/macro.cpp:435), pid=37385, tid=27651
>> #  assert(false) failed: unknown node on this path
>> 
>> 
>> when run with `-XX:-ProfileExceptionHandlers`. With different flag configurations this can be reproduced back as far as JDK 11. (See issue in JBS.)
>> 
>> The issue is caused during OSR compilation in macro expansion (`PhaseMacroExpand::eliminate_macro_nodes`):
>> 
>> During `eliminate_macro_nodes`, the boxing call `Integer.valueOf(var19)` (which can be seen in the graph below as node 359) is eliminated:
>> 
>> <img width="545" alt="Screenshot 2025-01-14 at 14 56 27" src="https://github.com/user-attachments/assets/233d0972-a477-4d7a-8cdc-adee3e670d45" />
>> 
>> Note that on the catch path we have MemBarReleaseLock node (462) whose control and memory input become top after 359 CallStaticJava is eliminated. This elimination is correct per se.
>> 
>> `PhaseMacroExpand::eliminate_macro_nodes` will then continue with the next macro node in its list, the allocation `new A()`, which it tries to eliminate using scalar replacement. Note that IGVN does not run in-between this macro elimination attempts.
>> 
>> During scalar replacement, while finding the values for each field and walking along the memory edges, 462 MemBarReleaseLock, whose memory input is still top, is hit in `PhaseMacroExpand::value_from_mem_phi`. `PhaseMacroExpand::value_from_mem_phi` does not expect top and the assert triggers.
>> 
>> A naive solution for this problem would be to run IGVN in between each macro elimination attempt in order to ensure the entire catch path with 462 MemBarReleaseLock is removed. This does indeed solve the problem but the performance impact of running IGVN in between each macro elimination is unclear.
>> 
>> Instead, we observe that `PhaseMacroExpan...
>
> Theo Weidmann has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update TestTopInMacroElimination.java

Thanks for adding the test! Some more minor comments.

test/hotspot/jtreg/compiler/macronodes/TestTopInMacroElimination.java line 25:

> 23: 
> 24: /*
> 25:  * @test

You should add the bug number:
Suggestion:

 * @test
 * @bug 8325030

test/hotspot/jtreg/compiler/macronodes/TestTopInMacroElimination.java line 29:

> 27:  * constructing new phis.
> 28:  * @modules java.base/jdk.internal.misc
> 29:  * @library /test/lib /

I think these are not required and can be removed:
Suggestion:

test/hotspot/jtreg/compiler/macronodes/TestTopInMacroElimination.java line 53:

> 51:                 for (int var8 = 0; var8 < 10000; ++var8) ;
> 52:             } catch (ArithmeticException a_e) {
> 53:             }

I suggest to add braces for the `for` loop:
Suggestion:

        for (int i = 0; i < 50; ++i) {
            try {
                synchronized (new A()) {
                    synchronized (TestTopInMacroElimination.class) {
                        for (int var19 = 0; var19 < Integer.valueOf(i2);) {
                            Integer.valueOf(var19);
                        }
                    }
                }
                for (int var8 = 0; var8 < 10000; ++var8) ;
            } catch (ArithmeticException a_e) {
            }
        }

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

PR Review: https://git.openjdk.org/jdk/pull/23104#pullrequestreview-2552054891
PR Review Comment: https://git.openjdk.org/jdk/pull/23104#discussion_r1916250325
PR Review Comment: https://git.openjdk.org/jdk/pull/23104#discussion_r1916249167
PR Review Comment: https://git.openjdk.org/jdk/pull/23104#discussion_r1916247744