RFR: 8327963: C2: fix construction of memory graph around Initialize node to prevent incorrect execution if allocation is removed [v6]

[Roland Westrelin]

On Thu, 15 May 2025 12:09:23 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> An `Initialize` node for an `Allocate` node is created with a memory
>> `Proj` of adr type raw memory. In order for stores to be captured, the
>> memory state out of the allocation is a `MergeMem` with slices for the
>> various object fields/array element set to the raw memory `Proj` of
>> the `Initialize` node. If `Phi`s need to be created during later
>> transformations from this memory state, The `Phi` for a particular
>> slice gets its adr type from the type of the `Proj` which is raw
>> memory. If during macro expansion, the `Allocate` is found to have no
>> use and so can be removed, the `Proj` out of the `Initialize` is
>> replaced by the memory state on input to the `Allocate`. A `Phi` for
>> some slice for a field of an object will end up with the raw memory
>> state on input to the `Allocate` node. As a result, memory state at
>> the `Phi` is incorrect and incorrect execution can happen.
>> 
>> The fix I propose is, rather than have a single `Proj` for the memory
>> state out of the `Initialize` with adr type raw memory, to use one
>> `Proj` per slice added to the memory state after the `Initalize`. Each
>> of the `Proj` should return the right adr type for its slice. For that
>> I propose having a new type of `Proj`: `NarrowMemProj` that captures
>> the right adr type.
>> 
>> Logic for the construction of the `Allocate`/`Initialize` subgraph is
>> tweaked so the right adr type captured in is own `NarrowMemProj` is
>> added to the memory sugraph. Code that removes an allocation or moves
>> it also has to be changed so it correctly takes the multiple memory
>> projections out of the `Initialize` node into account.
>> 
>> One tricky issue is that when EA split types for a scalar replaceable
>> `Allocate` node:
>> 
>> 1- the adr type captured in the `NarrowMemProj` becomes out of sync
>>   with the type of the slices for the allocation
>>   
>> 2- before EA, the memory state for one particular field out of the
>>   `Initialize` node can be used for a `Store` to the just allocated
>>   object or some other. So we can have a chain of `Store`s, some to
>>   the newly allocated object, some to some other objects, all of them
>>   using the state of `NarrowMemProj` out of the `Initialize`. After
>>   split unique types, the `NarrowMemProj` is for the slice of a
>>   particular allocation. So `Store`s to some other objects shouldn't
>>   use that memory state but the memory state before the `Allocate`.
>>   
>> For that, I added logic to update the adr type of `NarrowMemProj`
>> during split uni...
>
> Roland Westrelin has updated the pull request incrementally with 14 additional commits since the last revision:
> 
>  - typo
>  - more
>  - more
>  - more
>  - more
>  - more
>  - more
>  - more
>  - more
>  - review
>  - ... and 4 more: https://git.openjdk.org/jdk/compare/7afc47e4...af8480c0

I reworked this quite a bit.

1. In line with Emanuel's comment about making `_adr_type` `const` in `NarrowMemProj`, I changed the code in EA so, rather than update in place the `_adr_type` of the existing `NarrowMemProj`s, it creates new ones with the new `_adr_type`. That simplifies things because rather than replace existing `NarrowMemProj`s with new ones (which is what the update in place did in practice), the new code adds extra `NarrowMemProj`s and doesn't remove the existing ones. That, in turn, simplifies the logic elsewhere in EA because there's no need for the memory state that's not specific to a known allocations to be rewired around known allocations.

2. I added asserts to catch cases where `proj_out` is called but the node has more than one matching projection. With those asserts, I caught some false positive/cases where we got lucky and worked around them by reworking the code so it doesn't use `proj_out`. That's the case in `PhaseIdealLoop::intrinsify_fill()`: we can end up there with more than one `FramePtr` projection because the code pattern used elsewhere is to add one more projection and let identical projections common during igvn. Also in `PhaseIdealLoop::fix_ctrl_uses()`, the loop exit can have 3 projections in some cases. That's a transitory state before everything is wired correctly (a cases where we are lucky).

3. I tried to abstract away the many ways we iterate over projections in this patch.

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

PR Comment: https://git.openjdk.org/jdk/pull/24570#issuecomment-2883651987