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

Fri Oct 24 10:41:16 UTC 2025

On Mon, 29 Sep 2025 08:44:51 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 two additional commits since the last revision:
> 
>  - review
>  - Roberto's patches

Second batch of comments.

Will continue later at `class NarrowMemProjNode`.

src/hotspot/share/opto/library_call.cpp line 5612:

> 5610:       return MultiNode::CONTINUE;
> 5611:     };
> 5612:     init->apply_to_projs(move_proj, TypeFunc::Memory);

A "for each" using callback with `void` return would create a little less noise.

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

> 1607:       // null, this allocation does have an InitializeNode but this logic can't locate it (see comment in
> 1608:       // PhaseMacroExpand::initialize_object()).
> 1609:       MemBarNode* mb = MemBarNode::make(C, Op_MemBarStoreStore, Compile::AliasIdxRaw);

Would it not be nicer to have the explanation here than below, and refer from below to here? Would help the reading flow ;)

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

> 1636:       Node* ctrl = new ProjNode(init, TypeFunc::Control);
> 1637:       transform_later(ctrl);
> 1638:       Node* existing_raw_mem_proj = nullptr;

Tiny suggestion:
`existing_raw_mem_proj` -> `old_raw_mem_proj`, to emphasize that it is old, and will be replaced.

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

> 1644:         return MultiNode::CONTINUE;
> 1645:       };
> 1646:       init->apply_to_projs(find_raw_mem, TypeFunc::Memory);

A "for each" with `void` return callback would reduce noise.

src/hotspot/share/opto/memnode.cpp line 5468:

> 5466:   };
> 5467:   apply_to_projs(imax, i, replace_proj, TypeFunc::Memory);
> 5468: }

Ouff, it's a little sad that we modify the iterator both outside and inside the method call. But I don't have a better solution now either. It may just be what we have to do.

src/hotspot/share/opto/multnode.cpp line 63:

> 61:   };
> 62:   return apply_to_projs(find_proj, which_proj, is_io_use);
> 63: }

A `find_first` API with a boolean predicate could reduce some noise here.

src/hotspot/share/opto/multnode.cpp line 67:

> 65: template<class Callback> ProjNode* MultiNode::apply_to_projs(Callback callback, uint which_proj, bool is_io_use) const {
> 66:   auto filter = [&](ProjNode* proj) {
> 67:     if (proj->_is_io_use == is_io_use && callback(proj)) {

implicit zero check?

src/hotspot/share/opto/multnode.cpp line 81:

> 79:     return CONTINUE;
> 80:   };
> 81:   apply_to_projs(count_projs, which_proj);

for each pattern would have been nice here.

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

PR Review: https://git.openjdk.org/jdk/pull/24570#pullrequestreview-3375556080
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2459657397
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2459675950
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2459698388
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2459680660
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2459717105
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2459729036
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2459730207
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2459733750