RFR: 8362832: compiler/macronodes/TestTopInMacroElimination.java hits assert(false) failed: unexpected node

[Vladimir Kozlov]

On Mon, 20 Oct 2025 15:53:59 GMT, Benoît Maillard <bmaillard at openjdk.org> wrote:

> This PR prevents hitting an assert caused by encountering `top` while following the memory
> slice associated with a field when eliminating allocations in macro node elimination. This situation
> is the result of another elimination (boxing node elimination) that happened at the same
> macro expansion iteration.
> 
> ### Analysis
> 
> The issue appears in the macro expansion phase. We have a nested `synchronized` block,
> with the outer block synchronizing on `new A()` and the inner one on `TestTopInMacroElimination.class`.
> In the inner `synchronized` block we have an `Integer.valueOf` call in a loop.
> 
> In `PhaseMacroExpand::eliminate_boxing_node` we are getting rid of the `Integer.valueOf`
> call, as it is a non-escaping boxing node. After having eliminated the call,
> `PhaseMacroExpand::process_users_of_allocation` takes care of the users of the removed node.
> There, we replace usages of the fallthrough memory projection with `top`.
> 
> In the same macro expansion iteration, we later attempt to get rid of the `new A()` allocation
> in `PhaseMacroExpand::create_scalarized_object_description`. There, we have to make
> sure that all safepoints can still see the object fields as if the allocation was never deleted.
> For this, we attempt to find the last value on the slice of each specific field (`a`
> in this case). Because field `a` is never written to, and it is not explicitely initialized,
> there is no `Store` associated to it and not even a dedicated memory slice (we end up
> taking the `Bot` input on `MergeMem` nodes). By going up the memory chain, we eventually
> encounter the `MemBarReleaseLock` whose input was set to `top`. This is where the assert
> is hit.
> 
> ### Proposed Fix
> 
> In the end I opted for an analog fix as the similar [JDK-8325030](https://git.openjdk.org/jdk/pull/23104).
> If we get `top` from `scan_mem_chain` in `PhaseMacroExpand::value_from_mem`, then we can safely
> return `top` as well. This means that the safepoint will have `top` as data input, but this will
> eventually cleaned up by the next round of IGVN.
> 
> Another (tempting) option would have been to simply return `nullptr` from `PhaseMacroExpand::value_from_mem` when encoutering `top`. However this would result in bailing
> out from eliminating this allocation temporarily and effectively delaying it to a subsqequent
> macro expansion round.
> 
> 
> ### Testing
> - [x] [GitHub Actions](https://github.com/benoitmaillard/jdk/actions?query=branch%3AJDK-8362832)
> - [x] tier1-4, plus some internal testing
> 
> Thank you for reviewing!

I agree with fix.

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

Marked as reviewed by kvn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/27903#pullrequestreview-3361970213