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

[Emanuel Peter]

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

@rwestrel Thanks for your continued work on this, and you patience with the slow reviews 🙈 

I stumbled a bit over the many overloads of `apply_to_narrow_mem_projs`, and left some comments for that below.

There are really only one use of `already_has_narrow_mem_proj_with_adr_type` and one of `apply_to_narrow_mem_projs`, and you have a whole lot of methods that implement a lot of abstractions that confuse me. Is there a plan behind this, or just an artefact of many refactorings?

Could we not just implement those 2 methods directly using `apply_to_projs_any_iterator`?

I'll continue reviewing other parts now.

src/hotspot/share/opto/escape.cpp line 4457:

> 4455:           const TypePtr* new_adr_type = tinst->add_offset(adr_type->offset());
> 4456:           bool already_has_narrow_mem_proj_with_adr_type = init->already_has_narrow_mem_proj_with_adr_type(new_adr_type);
> 4457:           if (adr_type != new_adr_type && !already_has_narrow_mem_proj_with_adr_type) {

Nit: we could avoid the iteration inside `already_has_narrow_mem_proj_with_adr_type` if we did the `adr_type != new_adr_type` first. Feel free to ignore if you think this is a micro-optimization ;)

src/hotspot/share/opto/escape.cpp line 4467:

> 4465:           return MultiNode::CONTINUE;
> 4466:         };
> 4467:         init->apply_to_narrow_mem_projs(i, process_narrow_proj);

Is there a reason why this is not a `init->for_each_narrow_mem_proj(callback)`, that has an internal iterator?

Because with this API, I'm wondering: What would happen if I feed `apply_to_narrow_mem_projs` an iterator that does not belong to the `init`?

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

> 5489:   };
> 5490:   return apply_to_narrow_mem_projs(find_proj, adr_type) != nullptr;
> 5491: }

Am I seeing this right: `already_has_narrow_mem_proj_with_adr_type` calls `apply_to_narrow_mem_projs` with `callback = find_proj`. `find_proj` returns `BREAK_AND_RETURN_CURRENT_PROJ`, which is an element from `ApplyToProjs`. That would mean that when we call the `callback`, we get an enum element, and not a boolean, right?

If that is the case, you should probably not do an implicit comparison on this line:
`    if (proj->adr_type() == adr_type && callback(proj->as_NarrowMemProj())) {`
Hotspot style guide does not like implicit conversion. You should use an explicit comparison.

I think it would also be more clear what is happening. Currently, I'm a bit confused.

All the overloadings of `apply_to_narrow_mem_projs` make it a bit hard to see what goes where :/
I wonder if we really need all that complexity.

src/hotspot/share/opto/memnode.hpp line 1408:

> 1406:   template <class Callback, class Iterator> ProjNode* apply_to_narrow_mem_projs_any_iterator(Iterator i, Callback callback) const {
> 1407:     auto filter = [&](ProjNode* proj) {
> 1408:       if (proj->is_NarrowMemProj() && callback(proj->as_NarrowMemProj())) {

What does the `callback` return here? Are we sure this is not an implicit zero/null check, that the hotspot style guide would not be happy with?

src/hotspot/share/opto/memnode.hpp line 1424:

> 1422:   template <class Callback> ProjNode* apply_to_narrow_mem_projs(DUIterator& i, Callback callback) const {
> 1423:     return apply_to_narrow_mem_projs_any_iterator<Callback, UsesIterator>(UsesIterator(i, this), callback);
> 1424:   }

Do we really need to have a `public` API where we can pass in an iterator? What if someone uses it with an iterator for the wrong node?

The only use for it seems to be [ConnectionGraph::split_unique_types](https://github.com/openjdk/jdk/pull/24570/files#diff-03f7ae3cf79ff61be6e4f0590b7809a87825b073341fdbfcf36143b99c304474R4467) with `DUIterator`.

Is there a reason you want to pass the iterator explicitly?

src/hotspot/share/opto/memnode.hpp line 1428:

> 1426:   template <class Callback> ProjNode* apply_to_narrow_mem_projs(DUIterator_Fast& imax, DUIterator_Fast& i, Callback callback) const {
> 1427:     return apply_to_narrow_mem_projs_any_iterator<Callback, UsesIteratorFast>(UsesIteratorFast(imax, i, this), callback);
> 1428:   }

Is there any use for this method? I could not find any.

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

PR Review: https://git.openjdk.org/jdk/pull/24570#pullrequestreview-3375302849
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2459494172
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2459451788
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2459537600
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2459569501
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2459482037
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2459456185