RFR: 8327963: C2: fix construction of memory graph around Initialize node to prevent incorrect execution if allocation is removed [v8]
Emanuel Peter
epeter at openjdk.org
Tue May 27 09:15:01 UTC 2025
On Thu, 22 May 2025 15:20:14 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 one additional commit since the last revision:
>
> review
Thank you @rwestrel for taking this on! The solution seems very reasonable, using multiple projections. Thank you also for finding that extra test case that @robcasloz asked for, it makes sure that we have a reproducer that still works when @robcasloz makes changes that would make the other reproducers not reproduce any more ;)
I have plenty of small things below, nothing too big.
The largest source of confusion was the `apply_to_projs` construct.
I'll copy my comment from `apply_to_projs_any_iterator`:
----------------------
I was a little confused about this apply_to_proj construct. I this something we already use, a familiar concept, the apply_to?
> `// Iterate with i over all Proj uses calling callback`
Do we really iterate over all Proj uses? It seems that is not true if callback returns true, right?
This makes the semantic of the apply_to_projs a little subtle, right?
A suggestion: You could expect from Callback that it does not return true/false, but some enum value that describes what happens when you return that value. Maybe ApplyToProjs::CONTINUE vs ApplyToProjs::BREAK_AND_RETURN_CURRENT_PROJ?
src/hotspot/share/opto/escape.cpp line 4455:
> 4453: assert(init != nullptr, "can't find Initialization node for this Allocate node");
> 4454: DUIterator i = init->outs();
> 4455: auto process_narrow_proj = [tinst, init, this, igvn](NarrowMemProjNode* proj) {
In the style guide, it says:
> Prefer [&] as the capture list of a lambda expression.
https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md
There is a lengthy section on by-value capture, which I have not studied in depth. Probably what you are doing here is correct, but the style guide also talks about by-value capturing is "syntactically subtle". I guess you are only passing the lambda down, so there should be no issue... still it makes me a little nervous.
What do you think?
src/hotspot/share/opto/escape.cpp line 4467:
> 4465: set_map(proj, new_proj); // record it so ConnectionGraph::find_inst_mem() can find it
> 4466: }
> 4467: return false;
Can you add a in-line comment about what the `return false` means?
src/hotspot/share/opto/escape.cpp line 4804:
> 4802: assert(n->is_Initialize(), "We only push projections of Initialize");
> 4803: if (use->as_Proj()->_con == TypeFunc::Memory) { // Ignore precedent edge
> 4804: memnode_worklist.append_if_missing(use);
Do you know why we are using a `GrowableArray` here? Would a `UnikeNodeList` not serve us better since we are always doing `append_if_missing`, which essentially has to scan the whole `GrowableArray`?
src/hotspot/share/opto/library_call.cpp line 5568:
> 5566: int klass_idx = C->get_alias_index(ary_type->add_offset(oopDesc::klass_offset_in_bytes()));
> 5567: #endif
> 5568: auto move_proj = [=](ProjNode* proj) {
Same question about by-value capture here.
src/hotspot/share/opto/library_call.cpp line 5570:
> 5568: auto move_proj = [=](ProjNode* proj) {
> 5569: int alias_idx = C->get_alias_index(proj->adr_type());
> 5570: assert(alias_idx == Compile::AliasIdxRaw || alias_idx == elemidx || alias_idx == mark_idx || alias_idx == klass_idx, "should be raw memory or array element type");
Suggestion:
assert(alias_idx == Compile::AliasIdxRaw ||
alias_idx == elemidx ||
alias_idx == mark_idx ||
alias_idx == klass_idx, "should be raw memory or array element type");
Might be more readable.
src/hotspot/share/opto/macro.cpp line 1030:
> 1028: #endif
> 1029: init->replace_mem_projs_by(mem, &_igvn);
> 1030: assert(init->outcnt() == 0, "only a control and some memory projections expected");
I'm a little confused. Where should the control or memory projections be? I suppose not below `init`... Ah, are you trying to say that there only "should have been control and memory projections, which now are all removed"?
Suggestion:
assert(init->outcnt() == 0, "should only have had a control and some memory projections, and we removed them");
src/hotspot/share/opto/macro.cpp line 1647:
> 1645: transform_later(ctrl);
> 1646: Node* existing_raw_mem_proj = nullptr;
> 1647: auto find_raw_mem = [&, this](ProjNode* proj) {
Same question about lambda capture.
src/hotspot/share/opto/macro.cpp line 1657:
> 1655: Node* raw_mem_proj = new ProjNode(init, TypeFunc::Memory);
> 1656: transform_later(raw_mem_proj);
> 1657: assert(existing_raw_mem_proj != nullptr, "");
Suggestion:
init->apply_to_projs(find_raw_mem, TypeFunc::Memory);
assert(existing_raw_mem_proj != nullptr, "should have found it");
Node* raw_mem_proj = new ProjNode(init, TypeFunc::Memory);
transform_later(raw_mem_proj);
I would do the assert as early as possible.
src/hotspot/share/opto/memnode.cpp line 5483:
> 5481: return true;
> 5482: }
> 5483: return false;
Suggestion:
return proj->adr_type() == adr_type && callback(proj->as_NarrowMemProj();
src/hotspot/share/opto/memnode.hpp line 1411:
> 1409: return true;
> 1410: }
> 1411: return false;
Suggestion:
return proj->is_NarrowMemProj() && callback(proj->as_NarrowMemProj();
src/hotspot/share/opto/multnode.cpp line 48:
> 46: ProjNode* MultiNode::proj_out_or_null(uint which_proj) const {
> 47: assert((Opcode() != Op_If && Opcode() != Op_RangeCheck) || which_proj == (uint)true || which_proj == (uint)false, "must be 1 or 0");
> 48: assert(number_of_projs(which_proj) <= 1, "only when there's a single projection");
Does this hold for all `MultiNode`s under all circumstances? Or should we consider returning `nullptr` in this case?
src/hotspot/share/opto/multnode.cpp line 51:
> 49: auto find_proj = [which_proj, this](ProjNode* proj) {
> 50: assert((Opcode() != Op_If && Opcode() != Op_RangeCheck) || proj->Opcode() == (which_proj ? Op_IfTrue : Op_IfFalse),
> 51: "bad if #2");
What do you mean by `bad if #2`? Can you write something more descriptive?
src/hotspot/share/opto/multnode.cpp line 70:
> 68: return true;
> 69: }
> 70: return false;
Could not make a code suggestion unfortunately, as GitHub is blocked by the removal lines.
`return proj->_is_io_use == is_io_use && callback(proj);`
src/hotspot/share/opto/multnode.cpp line 257:
> 255: Compile::AliasType* atp = C->alias_type(_adr_type);
> 256: ciField* field = atp->field();
> 257: if (field) {
Suggestion:
if (field != nullptr) {
Style guide forbids implicit null checks.
src/hotspot/share/opto/multnode.hpp line 55:
> 53: uint number_of_projs(uint which_proj, bool is_io_use) const;
> 54:
> 55: // Run callback on all Proj projection from this node
Confused. Are there non-Proj projections?
src/hotspot/share/opto/multnode.hpp line 134:
> 132: }
> 133: return nullptr;
> 134: }
I was a little confused about this `apply_to_proj` construct. I this something we already use, a familiar concept, the `apply_to`?
Do we really iterate over `all` Proj uses? It seems that is not true if `callback` returns true, right?
This makes the semantic of the `apply_to_projs` a little subtle, right?
A suggestion: You could expect from `Callback` that it does not return `true/false`, but some `enum` value that describes what happens when you return that value. Maybe `ApplyToProjs::CONTINUE` vs `ApplyToProjs::BREAK_AND_RETURN_CURRENT_PROJ`?
src/hotspot/share/opto/multnode.hpp line 230:
> 228: return true;
> 229: }
> 230: return false;
Could also be simplified to a single line `return ...`
test/hotspot/jtreg/compiler/macronodes/TestEliminationOfAllocationWithoutUse.java line 2:
> 1: /*
> 2: * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
Is the copyright year accurate?
test/hotspot/jtreg/compiler/macronodes/TestInitializingStoreCapturing.java line 2:
> 1: /*
> 2: * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
Is the copyright year accurate?
-------------
Changes requested by epeter (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/24570#pullrequestreview-2870115629
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2108535409
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2108540079
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2108546984
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2108558177
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2108561165
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2108574535
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2108579647
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2108586458
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2108598966
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2108614731
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2108619844
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2108620767
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2108625866
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2108632723
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2108634420
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2108650897
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2108655508
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2108657963
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2108659208
More information about the hotspot-compiler-dev
mailing list