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

[Emanuel Peter]

On Mon, 27 Oct 2025 08:16:21 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)...
>
> Benoît Maillard has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update src/hotspot/share/opto/macro.cpp
>   
>   Co-authored-by: Daniel Lundén <daniel.lunden at oracle.com>

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

> 504:     } else if (mem->is_top()) {
> 505:       // The slice is on a dead path. Returning top prevents bailing out
> 506:       // from the elimination, and IGVN can later clean up.

You could make it more specific, and say what you say in your PR description:
`return nullptr` would lead to elimination bailout, but we want to prevent that. Just forwarding the `top` is also legal, and `IGVN` can just clean things up, and remove whatever receives top.

Does this mean that there could be paths that don't get `top`, and so for those paths it is nice that we are able to remove the allocation, right?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27903#discussion_r2477913239