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

[duke]

On Wed, 15 Jan 2025 09:54:22 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 three additional commits since the last revision:
> 
>  - Update test/hotspot/jtreg/compiler/macronodes/TestTopInMacroElimination.java
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>  - Update test/hotspot/jtreg/compiler/macronodes/TestTopInMacroElimination.java
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>  - Update test/hotspot/jtreg/compiler/macronodes/TestTopInMacroElimination.java
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>

@theoweidmannoracle 
Your change (at version 2b4434c1a7d1d6f6ad525a9acdd5b85fb3403f0e) is now ready to be sponsored by a Committer.

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

PR Comment: https://git.openjdk.org/jdk/pull/23104#issuecomment-2595003763