RFR: 8327963: [Umbrella] Incorrect result of C2 compiled code since JDK-8237581 [v2]
Emanuel Peter
epeter at openjdk.org
Wed Apr 23 12:38:59 UTC 2025
On Thu, 10 Apr 2025 15:56:25 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:
>
> TestIterativeEA fix
@rwestrel Thanks for looking into that. I vaguely remember working on https://github.com/openjdk/jdk/pull/18265 and being quite confused about the way we treat memory slices 😅
I think this seems like a good step in the right direction. Though I am a little unsure about the effect on `proj_out_or_null`, as you can see below. Maybe we can have a simple query that just checks `has_proj(TypeFunc::Memory)`? But there are also some cases where you actually use the projection, and I'm not sure if that means you might be missing some if there are multiple?
Like @merykitty I wonder if there are other cases where we have similar issues with memory slices. But I guess we should tackle those separately.
src/hotspot/share/opto/escape.cpp line 4123:
> 4121: result = result->in(MemNode::Memory);
> 4122: }
> 4123: if (!is_instance && result->Opcode() == Op_NarrowMemProj) {
Seems you are checking for `NarrowMemProj` and casting to it in multiple places. Why not enable the macro to do `is_...` and `as_...`?
src/hotspot/share/opto/escape.cpp line 4126:
> 4124: // Memory for non known instance can safely skip over a known instance allocation (that memory state doesn't access
> 4125: // the result of an allocation for a known instance).
> 4126: assert(result->as_Proj()->_con == TypeFunc::Memory, "a NarrowMemProj can only be a memory projection");
Can we verify that already in the `NarrowMemProj`, i.e. its constructor?
src/hotspot/share/opto/escape.cpp line 4128:
> 4126: assert(result->as_Proj()->_con == TypeFunc::Memory, "a NarrowMemProj can only be a memory projection");
> 4127: assert(toop != nullptr, "");
> 4128: Node *proj_in = result->in(0);
Suggestion:
Node* proj_in = result->in(0);
src/hotspot/share/opto/escape.cpp line 4460:
> 4458: for (DUIterator_Fast imax, i = init->fast_outs(imax); i < imax; i++) {
> 4459: ProjNode* proj = init->fast_out(i)->as_Proj();
> 4460: if (proj->Opcode() == Op_NarrowMemProj) {
Suggestion:
ProjNode* proj = init->fast_out(i)->as_NarrowMemProj();
if (proj != nullptr) {
That way you don't need to do the hacky cast below `((NarrowMemProjNode*)proj)`.
src/hotspot/share/opto/escape.cpp line 4465:
> 4463: if (adr_type != new_adr_type) {
> 4464: uint alias_ix = _compile->get_alias_index(new_adr_type);
> 4465: assert(_compile->get_general_index(alias_ix) == _compile->get_alias_index(adr_type), "new adr type should be narrowed down from existing adr type");
Suggestion:
DEBUG_ONLY( uint alias_idx = _compile->get_alias_index(new_adr_type); )
assert(_compile->get_general_index(alias_idx) == _compile->get_alias_index(adr_type), "new adr type should be narrowed down from existing adr type");
src/hotspot/share/opto/escape.cpp line 4469:
> 4467: ((NarrowMemProjNode*)proj)->set_adr_type(new_adr_type);
> 4468: igvn->hash_insert(proj);
> 4469: record_for_optimizer(proj);
That seems to be the only reason why we need the `_adr_type` non constant. How bad would it be if we just created a new node? Hmm maybe not worth it. Just wanted to know if you had considered it?
src/hotspot/share/opto/escape.cpp line 4742:
> 4740: if (n == nullptr) {
> 4741: continue;
> 4742: }
Could we now have multiple `NarrowMemProj`? If so, what would happen here?
src/hotspot/share/opto/escape.cpp line 4878:
> 4876: if (mem->Opcode() == Op_NarrowMemProj) {
> 4877: const TypePtr* at = mem->adr_type();
> 4878: uint idx = (uint) _compile->get_alias_index(at->is_ptr());
Suggestion:
uint alias_idx = (uint) _compile->get_alias_index(at->is_ptr());
for consistency with the other code
src/hotspot/share/opto/escape.cpp line 4885:
> 4883: }
> 4884: } else {
> 4885: // projection for a known allocation on a non known allocation slice: skip over the allocation
The thing about "non known" sounds like we should not be able to skip it... But I guess we still know they are from unrelated slices somehow?
src/hotspot/share/opto/graphKit.cpp line 3639:
> 3637: set_memory(_gvn.transform(new NarrowMemProjNode(init, TypeFunc::Memory, C->get_adr_type(mark_idx))), mark_idx);
> 3638: int klass_idx = C->get_alias_index(oop_type->add_offset(oopDesc::klass_offset_in_bytes()));
> 3639: set_memory(_gvn.transform(new NarrowMemProjNode(init, TypeFunc::Memory, C->get_adr_type(klass_idx))), klass_idx);
Hmm, so we now do have multiple projections for `TypeFunc::Memory`. That makes me a little nervous about `proj_out_or_null(TypeFunc::Memory)` elsewhere in your changes.
src/hotspot/share/opto/macro.cpp line 1022:
> 1020: #ifdef ASSERT
> 1021: Node* mem_proj = init->proj_out_or_null(TypeFunc::Memory);
> 1022: if (mem_proj != nullptr) {
What happens if there are multiple?
src/hotspot/share/opto/memnode.cpp line 5471:
> 5469: }
> 5470: }
> 5471: }
Can you please add a comment to the code why we need both variants?
src/hotspot/share/opto/multnode.hpp line 117:
> 115: return ProjNode::hash() + _adr_type->hash();
> 116: }
> 117: virtual bool cmp(const Node &n) const {
Suggestion:
virtual bool cmp(const Node& n) const {
src/hotspot/share/opto/multnode.hpp line 124:
> 122: }
> 123: public:
> 124: NarrowMemProjNode(Node *src, uint con, const TypePtr* adr_type)
Suggestion:
NarrowMemProjNode(Node* src, uint con, const TypePtr* adr_type)
src/hotspot/share/opto/multnode.hpp line 135:
> 133: }
> 134: virtual int Opcode() const;
> 135: };
Would it make sense to have these overridden? Just so you can print the `_adr_type` :)
#ifndef PRODUCT
virtual void dump_spec(outputStream *st) const;
virtual void dump_compact_spec(outputStream *st) const;
#endif
test/hotspot/jtreg/compiler/macronodes/TestEliminationOfAllocationWithoutUse.java line 26:
> 24: /*
> 25: * @test
> 26: * @bug 8327012
Suggestion:
* @bug 8327012 8327963
I would add the current bug id as well.
test/hotspot/jtreg/compiler/macronodes/TestEliminationOfAllocationWithoutUse.java line 39:
> 37: * @run main/othervm -Xcomp
> 38: * -XX:CompileCommand=compileonly,compiler.macronodes.TestEliminationOfAllocationWithoutUse::test*
> 39: * compiler.macronodes.TestEliminationOfAllocationWithoutUse
Would a run without Xcomp make sense?
test/hotspot/jtreg/compiler/macronodes/TestInitializingStoreCapturing.java line 26:
> 24: /*
> 25: * @test
> 26: * @bug 8327012
Suggestion:
* @bug 8327012 8327963
-------------
Changes requested by epeter (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/24570#pullrequestreview-2787011657
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2055891189
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2055892604
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2055893670
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2055900151
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2055906047
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2055909093
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2055911598
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2055914288
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2055919288
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2055923553
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2055926893
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2055933278
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2055934726
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2055875634
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2055878130
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2055939717
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2055940393
PR Review Comment: https://git.openjdk.org/jdk/pull/24570#discussion_r2055941586
More information about the hotspot-compiler-dev
mailing list