Integrated: 8325030: PhaseMacroExpand::value_from_mem_phi assert with "unknown node on this path"

[Theo Weidmann]

On Tue, 14 Jan 2025 14:20:37 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 `PhaseMacroExpand::value_from_mem_phi` tries to convert a memory phi to a value phi node. It is fine for a PhiNo...

This pull request has now been integrated.

Changeset: 4ad5d551
Author:    Theo Weidmann <tweidmann at openjdk.org>
URL:       https://git.openjdk.org/jdk/commit/4ad5d5514e8fccc17f7be900e008e83f09e34e97
Stats:     60 lines in 2 files changed: 59 ins; 0 del; 1 mod

8325030: PhaseMacroExpand::value_from_mem_phi assert with "unknown node on this path"

Reviewed-by: chagedorn, kvn

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

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