8327963: C2: fix construction of memory graph around Initialize node to prevent incorrect execution if allocation is removed #24570

rwestrel · 2025-04-10T11:39:36Z

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 Phis 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 Stores, 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 Stores 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 unique types and update the memory input of Stores that
don't depend on the memory state out of the allocation site.

I also wrote a verification pass to check that, in the memory graph,
nodes on a particular slice have the right adr type. That verification
pass is not included here because it uncovered issues that are
unrelated to this particular issue so I intend to propose it (with
fixes for those other issues) separately.

I reused the test cases that Emanuel included in
#18265 and went over all issues
linked to this one, tried the test cases and added the ones that I
could reproduce.

/cc hotspot-compiler

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8327963: C2: fix construction of memory graph around Initialize node to prevent incorrect execution if allocation is removed (Bug - P3)

Reviewers

Roberto Castañeda Lozano (@robcasloz - Reviewer) 🔄 Re-review required (review applies to 48257c91)

Contributors

Emanuel Peter <epeter@openjdk.org>
Roberto Castañeda Lozano <rcastanedalo@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24570/head:pull/24570
$ git checkout pull/24570

Update a local copy of the PR:
$ git checkout pull/24570
$ git pull https://git.openjdk.org/jdk.git pull/24570/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24570

View PR using the GUI difftool:
$ git pr show -t 24570

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24570.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-04-10T11:40:25Z

👋 Welcome back roland! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-04-10T11:41:12Z

@rwestrel This change is no longer ready for integration - check the PR body for details.

openjdk · 2025-04-10T11:41:58Z

@rwestrel
The hotspot-compiler label was successfully added.

rwestrel · 2025-04-10T11:44:00Z

/contributor add @eme64

openjdk · 2025-04-10T11:45:43Z

@rwestrel
Contributor Emanuel Peter <epeter@openjdk.org> successfully added.

mlbridge · 2025-04-10T11:54:19Z

Webrevs

merykitty · 2025-04-10T12:29:20Z

It would be great if we have union memory slices for this.

rwestrel · 2025-04-14T15:35:17Z

It would be great if we have union memory slices for this.

Something like that would fix it but it would be trickier to get right that this point fix, I think. Do you see any other use for it?

test/hotspot/jtreg/compiler/macronodes/TestEliminationOfAllocationWithoutUse.java

merykitty · 2025-04-17T06:16:12Z

@rwestrel There are other places in C2 when the memory slices are not expressive enough, which leads us to wrapping those accesses in CPUMemBar because otherwise the memory edges would be incorrect. This would become more prominent in Valhalla as atomic flat accesses try to write multiple fields at once.

https://github.com/openjdk/valhalla/blob/6b476fa7d67bf80591ed86d291b167d3d5ee5c28/src/hotspot/share/opto/inlinetypenode.cpp#L855

eme64

@rwestrel Thanks for looking into that. I vaguely remember working on #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/multnode.hpp

eme64 · 2025-04-23T11:50:45Z

src/hotspot/share/opto/multnode.hpp

+    _adr_type = adr_type;
+  }
+  virtual int Opcode() const;
+};


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

Ah, or does it already get printed from the adr_type() i.e. the virtual method?

Hmm, no I don't think so... right?

Done in new commit.

eme64 · 2025-04-23T11:59:12Z

src/hotspot/share/opto/escape.cpp

      }
      result = result->in(MemNode::Memory);
    }
+    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_...?

Done in new commit.

eme64 · 2025-04-23T12:00:04Z

src/hotspot/share/opto/escape.cpp

+    if (!is_instance && result->Opcode() == Op_NarrowMemProj) {
+      // Memory for non known instance can safely skip over a known instance allocation (that memory state doesn't access
+      // the result of an allocation for a known instance).
+      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?

TypeFunc::Memory is now hardcoded in the NarrowMemProj constructor.

src/hotspot/share/opto/escape.cpp

eme64 · 2025-04-23T12:24:48Z

src/hotspot/share/opto/memnode.cpp

+void InitializeNode::replace_mem_projs_by(Node* mem, Compile* C) {
+  for (DUIterator_Fast imax, i = fast_outs(imax); i < imax; i++) {
+    ProjNode* proj = fast_out(i)->as_Proj();
+    if (proj->_con == TypeFunc::Memory) {
+      C->gvn_replace_by(proj, mem);
+    }
+  }
+}
+
+void InitializeNode::replace_mem_projs_by(Node* mem, PhaseIterGVN* igvn) {
+  for (DUIterator_Fast imax, i = fast_outs(imax); i < imax; i++) {
+    ProjNode* proj = fast_out(i)->as_Proj();
+    if (proj->_con == TypeFunc::Memory) {
+      igvn->replace_node(proj, mem);
+      --i; --imax;
+    }
+  }
+}


Can you please add a comment to the code why we need both variants?

src/hotspot/share/opto/multnode.hpp

test/hotspot/jtreg/compiler/macronodes/TestEliminationOfAllocationWithoutUse.java

eme64 · 2025-04-23T12:29:06Z

test/hotspot/jtreg/compiler/macronodes/TestEliminationOfAllocationWithoutUse.java

+ * @run main/othervm -Xcomp -XX:-TieredCompilation
+ *                   -XX:CompileCommand=compileonly,compiler.macronodes.TestEliminationOfAllocationWithoutUse::test*
+ *                   compiler.macronodes.TestEliminationOfAllocationWithoutUse
+ * @run main/othervm -Xcomp
+ *                   -XX:CompileCommand=compileonly,compiler.macronodes.TestEliminationOfAllocationWithoutUse::test*
+ *                   compiler.macronodes.TestEliminationOfAllocationWithoutUse


Would a run without Xcomp make sense?

Some test methods run a loop for only a few iterations, not sure a run without -Xcomp makes much sense.

test/hotspot/jtreg/compiler/macronodes/TestInitializingStoreCapturing.java

eme64 · 2025-04-23T12:36:31Z

@rwestrel You should update the bug title, it just sounds too generic. It is really helpful when the "blame" history gives you a helpful comment rather than Umbrella or incorrect result or some bug numbers.

I suggest Use NarrowMemProj to project / split memory slice after Initialize, but you probably have an even better idea ;)

…tionWithoutUse.java Co-authored-by: Andrey Turbanov <turbanoff@gmail.com>

Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

…pturing.java Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

…tionWithoutUse.java Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

robcasloz · 2025-09-19T12:35:15Z

src/hotspot/share/opto/multnode.cpp

+
+void NarrowMemProjNode::dump_spec(outputStream *st) const {
+  ProjNode::dump_spec(st);
+  dump_adr_type(st);


Do we need to define a special version of NarrowMemProjNode::dump_adr_type or could we just have the same effect calling MemNode::dump_adr_type(this, _adr_type, st) here?

robcasloz · 2025-09-19T12:35:22Z

src/hotspot/share/opto/multnode.cpp

+
+void NarrowMemProjNode::dump_compact_spec(outputStream *st) const {
+  ProjNode::dump_compact_spec(st);
+  dump_adr_type(st);


test/hotspot/jtreg/compiler/macronodes/TestEliminationOfAllocationWithoutUse.java

src/hotspot/share/opto/multnode.hpp

src/hotspot/share/opto/macro.cpp

robcasloz · 2025-09-19T13:00:24Z

test/hotspot/jtreg/compiler/macronodes/TestEarlyEliminationOfAllocationWithoutUse.java

Please add a package declaration (and make the corresponding class names fully qualified in the @run directives).

robcasloz · 2025-09-19T13:02:43Z

test/hotspot/jtreg/compiler/escapeAnalysis/TestIterativeEA.java

+    analyzer.shouldContain("++++ Eliminated: 51 Allocate");
+    analyzer.shouldContain("++++ Eliminated: 84 Allocate");


Did you analyze why there are more allocations removed than before in this test case? I did not expect this changeset to have an effect on the number of removed allocations.

There are not more allocations removed. The message is confusing.
"Eliminated: 84 Allocate" logs that node number 84 was eliminated (and not 84 nodes).
This patch changes the number of nodes required at allocations so it also has an impact on node numbering.

I see, thanks. Expecting specific C2 node identifiers seems fragile. I understand it is a pre-existing issue, but since this changeset needs to address it anyway, please consider making it more robust by e.g. using regular expression matching. Here is a suggestion, feel free to incorporate it: 9fd6378. The ultimately improvement would be using the IR test framework, but that is out of scope here.

robcasloz · 2025-09-19T13:07:09Z

src/hotspot/share/opto/escape.hpp

                        // Memory Phi    - most recent unique Phi split out
                        //                 from this Phi
                        // MemNode       - new memory input for this node
-                        // ChecCastPP    - allocation that this is a cast of
+                        // CheckCastPP   - allocation that this is a cast of
                        // allocation    - CheckCastPP of the allocation


Please add a new entry here explaining how _node_map is used for NarrowMemProjNode nodes.

…tionWithoutUse.java Co-authored-by: Roberto Castañeda Lozano <robcasloz@users.noreply.github.com>

Co-authored-by: Roberto Castañeda Lozano <robcasloz@users.noreply.github.com>

rwestrel · 2025-09-22T13:32:43Z

@robcasloz thanks for the review. New commit addresses most of your comments.

robcasloz

Thanks for addressing my comments, Roland. I have a couple of follow-up questions. I also realized that we need to adjust IGV's custom logic to schedule the new projection nodes more accurately and combine them into their parent nodes when using the "Condense graph" filter. Please consider incorporating the following patch into this changeset: 63a536a.

robcasloz · 2025-09-24T12:14:50Z

src/hotspot/share/opto/macro.cpp

      // elimination. Simply add the MemBarStoreStore after object
      // initialization.
-      MemBarNode* mb = MemBarNode::make(C, Op_MemBarStoreStore, Compile::AliasIdxBot);
+      MemBarNode* mb = MemBarNode::make(C, Op_MemBarStoreStore, Compile::AliasIdxRaw);


Thanks for adding the comment. A follow-up question: the full comment below makes the argument that re-ordering by the compiler can't happen by construction because a later Store that publishes the just allocated object reference is indirectly control dependent on the Initialize node. However, in this case, there may be no such Initialize node (init == nullptr || init->req() < InitializeNode::RawStores). I assume the memory barrier relaxation is still OK in this scenario because we cannot have later, publishing stores of the allocated object reference? That is, if there exists such a store then there must necessarily exist an Initialize node? Or is there any other reason I am missing? It would be good to clarify this point in the comment.

rwestrel · 2025-09-29T08:38:43Z

/contributor add @robcasloz

rwestrel · 2025-09-29T08:40:01Z

@robcasloz Thanks for the patches. I added them.

openjdk · 2025-09-29T08:41:20Z

@rwestrel
Contributor Roberto Castañeda Lozano <rcastanedalo@openjdk.org> successfully added.

robcasloz

Thanks for and for addressing all comments and questions, Roland. Looks good!

robcasloz · 2025-09-29T14:14:16Z

src/hotspot/share/opto/macro.cpp

      // elimination. Simply add the MemBarStoreStore after object
      // initialization.
-      MemBarNode* mb = MemBarNode::make(C, Op_MemBarStoreStore, Compile::AliasIdxBot);
+      MemBarNode* mb = MemBarNode::make(C, Op_MemBarStoreStore, Compile::AliasIdxRaw);


Thanks for the clarification and the pointer to the comment in initialize_object, that helps.

robcasloz · 2025-10-20T18:55:30Z

Hi @rwestrel, could you please have a look at the merge conflicts of this PR so that we can progress further with the review of this work?

robcasloz · 2025-10-21T07:41:37Z

Hi @rwestrel, could you please have a look at the merge conflicts of this PR so that we can progress further with the review of this work?

The conflict is caused by the integration of JDK-8360031, which relaxes the assertion in

jdk/src/hotspot/share/opto/memnode.cpp

Line 4232 in 430041d

assert(outcnt() > 0 && outcnt() <= 2, "Only one or two out edges allowed");

which is also touched by this changeset. Is the current assertion in mainline (after JDK-8360031) still valid in the context of this changeset?

robcasloz · 2025-10-21T13:49:12Z

Is the current assertion in mainline (after JDK-8360031) still valid in the context of this changeset?

I did a bit of testing and updating the asserted invariant to (outcnt() > 0 && outcnt() <= 2) || Opcode() == Op_Initialize seems to work.

eme64

@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.

eme64 · 2025-10-24T09:12:43Z

src/hotspot/share/opto/escape.cpp

+          }
+          return MultiNode::CONTINUE;
+        };
+        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?

If it was a plain for_each_narrow_mem_proj, your callback would also not have to return anything, you could remove this line: return MultiNode::CONTINUE;.

eme64 · 2025-10-24T09:13:50Z

src/hotspot/share/opto/memnode.hpp

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


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

eme64 · 2025-10-24T09:23:19Z

src/hotspot/share/opto/memnode.hpp

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


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 with DUIterator.

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

Also: why does it return anything? What kind of callback is exected here? I think the public API deserves a bit of documentation.

The use in ConnectionGraph::split_unique_types really does not need a return value, and also could work fine with a callback that return void.

eme64 · 2025-10-24T09:26:13Z

src/hotspot/share/opto/escape.cpp

+          bool already_has_narrow_mem_proj_with_adr_type = init->already_has_narrow_mem_proj_with_adr_type(new_adr_type);
+          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 ;)

eme64 · 2025-10-24T09:36:11Z

src/hotspot/share/opto/memnode.cpp

+template<class Callback> ProjNode* InitializeNode::apply_to_narrow_mem_projs(Callback callback, const TypePtr* adr_type) const {
+  auto filter = [&](NarrowMemProjNode* proj) {
+    if (proj->adr_type() == adr_type && callback(proj->as_NarrowMemProj())) {
+      return BREAK_AND_RETURN_CURRENT_PROJ;
+    }
+    return CONTINUE;
+  };
+  return apply_to_narrow_mem_projs(filter);
+}
+
+bool InitializeNode::already_has_narrow_mem_proj_with_adr_type(const TypePtr* adr_type) const {
+  auto find_proj = [](ProjNode* proj) {
+    return BREAK_AND_RETURN_CURRENT_PROJ;
+  };
+  return apply_to_narrow_mem_projs(find_proj, adr_type) != nullptr;
+}


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.

eme64 · 2025-10-24T09:44:55Z

src/hotspot/share/opto/memnode.hpp

+  // Iterate with i over all NarrowMemProj uses calling callback
+  template <class Callback, class Iterator> ProjNode* apply_to_narrow_mem_projs_any_iterator(Iterator i, Callback callback) const {
+    auto filter = [&](ProjNode* proj) {
+      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?

eme64

Second batch of comments.

Will continue later at class NarrowMemProjNode.

eme64 · 2025-10-24T10:10:38Z

src/hotspot/share/opto/library_call.cpp

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


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

eme64 · 2025-10-24T10:16:29Z

src/hotspot/share/opto/macro.cpp

+      // See comment below in this if's other branch that explains why a raw memory MemBar is good enough. If init is
+      // null, this allocation does have an InitializeNode but this logic can't locate it (see comment in
+      // PhaseMacroExpand::initialize_object()).
+      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 ;)

eme64 · 2025-10-24T10:18:04Z

src/hotspot/share/opto/macro.cpp

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


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

Alternatives:

init->unique_raw_mem_proj()

init->unique_out(raw_mem_proj_predicate) with boolean predicate.
Both of those would assert if we find multiple or none.

eme64 · 2025-10-24T10:23:26Z

src/hotspot/share/opto/macro.cpp

      transform_later(ctrl);
-      Node* mem = new ProjNode(init, TypeFunc::Memory);
-      transform_later(mem);
+      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.

eme64 · 2025-10-24T10:29:33Z

src/hotspot/share/opto/memnode.cpp

+  DUIterator_Fast imax, i = fast_outs(imax);
+  auto replace_proj = [&](ProjNode* proj) {
+    igvn->replace_node(proj, mem);
+    --i; --imax;
+    return CONTINUE;
+  };
+  apply_to_projs(imax, i, replace_proj, TypeFunc::Memory);
+}


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.

eme64 · 2025-10-24T10:33:36Z

src/hotspot/share/opto/multnode.cpp

+  auto find_proj = [&](ProjNode* proj) {
+    assert((Opcode() != Op_If && Opcode() != Op_RangeCheck) || proj->Opcode() == (which_proj ? Op_IfTrue : Op_IfFalse),
+           "incorrect projection node at If/RangeCheck: IfTrue on false path or IfFalse on true path");
+    return BREAK_AND_RETURN_CURRENT_PROJ;
+  };
+  return apply_to_projs(find_proj, which_proj);
 }

 ProjNode* MultiNode::proj_out_or_null(uint which_proj, bool is_io_use) const {
-  for (DUIterator_Fast imax, i = fast_outs(imax); i < imax; i++) {
-    ProjNode* proj = fast_out(i)->isa_Proj();
-    if (proj != nullptr && (proj->_con == which_proj) && (proj->_is_io_use == is_io_use)) {
-      return proj;
+  assert(number_of_projs(which_proj, is_io_use) <= 1, "only when there's a single projection");
+  auto find_proj = [](ProjNode* proj) {
+    return BREAK_AND_RETURN_CURRENT_PROJ;
+  };
+  return apply_to_projs(find_proj, which_proj, is_io_use);
+}


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

eme64 · 2025-10-24T10:34:01Z

src/hotspot/share/opto/multnode.cpp

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


implicit zero check?

eme64 · 2025-10-24T10:35:17Z

src/hotspot/share/opto/multnode.cpp

+  auto count_projs = [&](ProjNode* proj) {
+    cnt++;
+    return CONTINUE;
+  };
+  apply_to_projs(count_projs, which_proj);


for each pattern would have been nice here.

eme64

Last batch of comments for this round.

I'm quite happy with the solution @rwestrel , thanks for coming up with this 😊

I think really all my suggestions / requests are about the complexity / implementation around the apply_... methods. It seems to me that it makes sense to internally implement it with the apply_... functionality that take callbacks which return a ApplyToProjs. But I think many of the public API methods could have a simpler semantic, for example many could just be for_each_... calls that just take a callback with void return / no return.

eme64 · 2025-10-24T13:18:46Z

src/hotspot/share/opto/multnode.hpp

+    return sizeof(*this);
+  }
+public:
+  NarrowMemProjNode(Node* src, const TypePtr* adr_type)


Can you feed it any other src than a InitializeNode*?

Suggested change

NarrowMemProjNode(Node* src, const TypePtr* adr_type)

NarrowMemProjNode(InitializeNode* src, const TypePtr* adr_type)

eme64 · 2025-10-24T13:21:04Z

src/hotspot/share/opto/multnode.hpp

+#endif
+};
+
+template <class Callback> ProjNode* MultiNode::apply_to_projs(DUIterator_Fast& imax, DUIterator_Fast& i, Callback callback, uint which_proj) const {


Does this not belong right after the MultiNode? Or even in multnode.cpp?

It needs the ProjNode declaration because it accesses proj->_con and can't be in multnode.cpp because of the template parameter.

eme64 · 2025-10-24T13:22:44Z

src/hotspot/share/opto/multnode.hpp

+
+template <class Callback> ProjNode* MultiNode::apply_to_projs(DUIterator_Fast& imax, DUIterator_Fast& i, Callback callback, uint which_proj) const {
+  auto filter = [&](ProjNode* proj) {
+    if (proj->_con == which_proj && callback(proj)) {


Implicit zero check?

eme64 · 2025-10-24T13:23:48Z

src/hotspot/share/opto/phaseX.cpp

+  auto enqueue_init_mem_projs = [&](ProjNode* proj) {
+    add_users_to_worklist0(proj, worklist);
+    return MultiNode::CONTINUE;
+  };


for_each call below would mean we would not need to return anything.

eme64 · 2025-10-24T13:31:16Z

src/hotspot/share/opto/multnode.cpp

+void NarrowMemProjNode::dump_spec(outputStream *st) const {
+  ProjNode::dump_spec(st);
+  MemNode::dump_adr_type(_adr_type, st);
+}
+
+void NarrowMemProjNode::dump_compact_spec(outputStream *st) const {
+  ProjNode::dump_compact_spec(st);
+  MemNode::dump_adr_type(_adr_type, st);
+}


Can you show us an example out put of dump? I'm just wondering if there maybe needs to be a space between the two, and if it is immediately readable :)

Actually, Node::dump already takes care of dumping the adr_type. So I removed the dump methods from NarrowMemProjNode. Here is an example output:

59 Initialize === 83 1 62 1 1 1 121 [[ 124 123 63 64 65 ]] !jvms: TestInitializingStoreCapturing::testInitializeArray @ bci:1 (line 57) TestInitializingStoreCapturing::testInitializeArray @ bci:1 (line 57) 63 NarrowMemProj === 59 [[ 15 ]] #2 Memory: @java/lang/Object *, idx=4; !jvms: TestInitializingStoreCapturing::testInitializeArray @ bci:1 (line 57) 64 NarrowMemProj === 59 [[ 15 ]] #2 Memory: @java/lang/Object+8 * [narrowklass], idx=5; !jvms: TestInitializingStoreCapturing::testInitializeArray @ bci:1 (line 57) 65 NarrowMemProj === 59 [[ 15 ]] #2 Memory: @float[int:>=0] (java/lang/Cloneable,java/io/Serializable):exact+any *, idx=6; !jvms: TestInitializingStoreCapturing::testInitializeArray @ bci:1 (line 57) 66 CheckCastPP === 125 86 [[ 79 ]] #float[int:1] (java/lang/Cloneable,java/io/Serializable):NotNull:exact * !jvms: TestInitializingStoreCapturing::testInitializeArray @ bci:1 (line 57)

eme64 · 2025-10-24T13:33:40Z

src/hotspot/share/opto/memnode.cpp

+}
+
+
+template<class Callback> ProjNode* InitializeNode::apply_to_narrow_mem_projs(Callback callback, const TypePtr* adr_type) const {


Another nit: we will only ever return a NarrowMemProj, so you might as well make the return value more precise ;)

rwestrel · 2025-10-28T13:35:59Z

@eme64 I pushed a new commit which should address all your comments.

eme64

@rwestrel Thanks for the updates, it already looks better :)

I had a few minutes to look over the apply_.. solutions. I left a few comments, and hope that we can make the code just a little slicker still ;)

eme64 · 2025-10-28T16:55:39Z

src/hotspot/share/opto/memnode.hpp

+  // Run callback on all NarrowMem proj uses using passed iterator
+  template <class Callback> NarrowMemProjNode* apply_to_narrow_mem_projs(DUIterator& i, Callback callback) const {
+    return apply_to_narrow_mem_projs_any_iterator<Callback, UsesIterator>(UsesIterator(i, this), callback);
+  }


Is this one still needed?

It also seems that the upper two can be merged. Maybe all "overloadings" of apply_to_narrow_mem_projs can be merged, no? Or are there really multiple uses?

Basically we could call apply_to_narrow_mem_projs_any_iterator directly from the two uses:

already_has_narrow_mem_proj_with_adr_type

for_each_narrow_mem_proj_with_new_uses

eme64 · 2025-10-28T16:58:09Z

src/hotspot/share/opto/memnode.cpp

+template <class Callback> NarrowMemProjNode* InitializeNode::apply_to_narrow_mem_projs(Callback callback) const {
+  DUIterator_Fast imax, i = fast_outs(imax);
+  return apply_to_narrow_mem_projs_any_iterator(UsesIteratorFast(imax, i, this), callback);
+}
+
+
+template<class Callback> NarrowMemProjNode* InitializeNode::apply_to_narrow_mem_projs(Callback callback, const TypePtr* adr_type) const {
+  auto filter = [&](NarrowMemProjNode* proj) {
+    if (proj->adr_type() == adr_type && callback(proj->as_NarrowMemProj()) == BREAK_AND_RETURN_CURRENT_PROJ) {
+      return BREAK_AND_RETURN_CURRENT_PROJ;
+    }
+    return CONTINUE;
+  };
+  return apply_to_narrow_mem_projs(filter);
+}


It seems to me that the upper method is only used by the lower here. Why not just collapse them? It would also reduce the "overloading noise".

eme64 · 2025-10-28T17:07:16Z

src/hotspot/share/opto/multnode.hpp

+  // Run callback on projections with iterator passed as argument
+  template<class Callback> ProjNode* apply_to_projs(DUIterator_Fast& imax, DUIterator_Fast& i, Callback callback) const {
+    return apply_to_projs_any_iterator<Callback, UsesIteratorFast>(UsesIteratorFast(imax, i, this), callback);
+  }
+
+  // Same but with default iterator
+  template<class Callback> ProjNode* apply_to_projs(Callback callback) const {
+    DUIterator_Fast imax, i = fast_outs(imax);
+    return apply_to_projs(imax, i, callback);
+  }
+
+  // Same but only for Proj node whose _con matches which_proj
+  template <class Callback> ProjNode* apply_to_projs(DUIterator_Fast& imax, DUIterator_Fast& i, Callback callback, uint which_proj) const;
+
+  // Same but with default iterator
+  template<class Callback> ProjNode* apply_to_projs(Callback callback, uint which_proj) const {
+    DUIterator_Fast imax, i = fast_outs(imax);
+    return apply_to_projs(imax, i, callback, which_proj);
+  }
+
+  // Same but for matching _con and _is_io_use
+  template <class Callback> ProjNode* apply_to_projs(Callback callback, uint which_proj, bool is_io_use) const;


Do these need to be public? Or could they be protected, so they are only available to subtypes?

And do we really need all the variants of apply_to_projs, or could we collapse them a little?

It is just a lot of boilerplate, would be nice if it was a little slicker ;)

eme64 and others added 5 commits April 10, 2025 12:54

8327012

9f220f4

test4

5f8e5bf

initializing store capturing: test stub

5041f64

more tests, WIP

dfa5da2

fix & extra tests

125f970

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Apr 10, 2025

whitespace

71fd831

rwestrel marked this pull request as ready for review April 10, 2025 11:48

openjdk bot added the rfr Pull request is ready for review label Apr 10, 2025

TestIterativeEA fix

a4031f3

turbanoff reviewed Apr 16, 2025

View reviewed changes

test/hotspot/jtreg/compiler/macronodes/TestEliminationOfAllocationWithoutUse.java Outdated Show resolved Hide resolved

eme64 suggested changes Apr 23, 2025

View reviewed changes

rwestrel and others added 8 commits April 24, 2025 11:15

Update test/hotspot/jtreg/compiler/macronodes/TestEliminationOfAlloca…

c32e453

…tionWithoutUse.java Co-authored-by: Andrey Turbanov <turbanoff@gmail.com>

Update src/hotspot/share/opto/multnode.hpp

082dcb5

Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

Update src/hotspot/share/opto/escape.cpp

7139deb

Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

Update src/hotspot/share/opto/multnode.hpp

469d8a5

Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

Update test/hotspot/jtreg/compiler/macronodes/TestInitializingStoreCa…

1639264

…pturing.java Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

Update test/hotspot/jtreg/compiler/macronodes/TestEliminationOfAlloca…

377a8d7

…tionWithoutUse.java Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

Update src/hotspot/share/opto/escape.cpp

a32b1d2

Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

Update src/hotspot/share/opto/escape.cpp

7afc47e

Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

robcasloz suggested changes Sep 19, 2025

View reviewed changes

rwestrel and others added 8 commits September 22, 2025 13:53

Update test/hotspot/jtreg/compiler/macronodes/TestEliminationOfAlloca…

c64d68c

…tionWithoutUse.java Co-authored-by: Roberto Castañeda Lozano <robcasloz@users.noreply.github.com>

Update src/hotspot/share/opto/multnode.hpp

0ff5c45

Co-authored-by: Roberto Castañeda Lozano <robcasloz@users.noreply.github.com>

Update src/hotspot/share/opto/multnode.hpp

46972dc

Co-authored-by: Roberto Castañeda Lozano <robcasloz@users.noreply.github.com>

Update src/hotspot/share/opto/graphKit.cpp

917ea91

Co-authored-by: Roberto Castañeda Lozano <robcasloz@users.noreply.github.com>

Update src/hotspot/share/opto/graphKit.cpp

2207487

Co-authored-by: Roberto Castañeda Lozano <robcasloz@users.noreply.github.com>

Update src/hotspot/share/opto/macro.cpp

f871390

Co-authored-by: Roberto Castañeda Lozano <robcasloz@users.noreply.github.com>

Update src/hotspot/share/opto/macro.cpp

6ea8c81

Co-authored-by: Roberto Castañeda Lozano <robcasloz@users.noreply.github.com>

review

9fd8dc1

robcasloz added a commit to robcasloz/jdk that referenced this pull request Sep 24, 2025

Apply openjdk#24570

2f9f3d7

robcasloz suggested changes Sep 24, 2025

View reviewed changes

rwestrel added 2 commits September 29, 2025 10:10

Roberto's patches

f12efbc

review

48257c9

robcasloz approved these changes Sep 29, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 29, 2025

eme64 reviewed Oct 24, 2025

View reviewed changes

rwestrel added 2 commits October 28, 2025 11:19

Merge branch 'master' into JDK-8327963

b5ac753

review

957be06

openjdk bot removed the ready Pull request is ready to be integrated label Oct 28, 2025

eme64 reviewed Oct 28, 2025

View reviewed changes

		analyzer.shouldContain("++++ Eliminated: 51 Allocate");
		analyzer.shouldContain("++++ Eliminated: 84 Allocate");

		bool already_has_narrow_mem_proj_with_adr_type = init->already_has_narrow_mem_proj_with_adr_type(new_adr_type);
		if (adr_type != new_adr_type && !already_has_narrow_mem_proj_with_adr_type) {

	NarrowMemProjNode(Node* src, const TypePtr* adr_type)
	NarrowMemProjNode(InitializeNode* src, const TypePtr* adr_type)

		}


		template<class Callback> ProjNode* InitializeNode::apply_to_narrow_mem_projs(Callback callback, const TypePtr* adr_type) const {

8327963: C2: fix construction of memory graph around Initialize node to prevent incorrect execution if allocation is removed #24570

Are you sure you want to change the base?

8327963: C2: fix construction of memory graph around Initialize node to prevent incorrect execution if allocation is removed #24570

Uh oh!

Conversation

rwestrel commented Apr 10, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Contributors

Reviewing

Uh oh!

bridgekeeper bot commented Apr 10, 2025

Uh oh!

openjdk bot commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Apr 10, 2025

Uh oh!

rwestrel commented Apr 10, 2025

Uh oh!

openjdk bot commented Apr 10, 2025

Uh oh!

mlbridge bot commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

merykitty commented Apr 10, 2025

Uh oh!

rwestrel commented Apr 14, 2025

Uh oh!

Uh oh!

merykitty commented Apr 17, 2025

Uh oh!

eme64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eme64 commented Apr 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwestrel commented Apr 10, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Apr 10, 2025 •

edited

Loading

mlbridge bot commented Apr 10, 2025 •

edited

Loading