8340214: C2 compilation asserts with "no node with a side effect" in PhaseIdealLoop::try_sink_out_of_loop #21303

rwestrel · 2024-10-02T11:21:43Z

The patch includes 2 test cases for this: test1() causes the assert
failure in the bug description, test2() causes an incorrect execution
where a load floats above a store that it should be dependent on.

In the test cases, field is accessed on object a of type A. When
the field is accessed, the type that c2 has for a is A with
interface I. The holder of the field is class A which implements
no interface. The reason the type of a and the type of the holder
are slightly different is because a is the result of a merge of
objects of subclasses B and C which implements I.

The root cause of the bug is that Compile::flatten_alias_type()
doesn't change A + interface I into A, the actual holder of the
field. So field in A + interface I and field in A get
different slices which is wrong. At parse time, the logic that creates
the Store node uses:

C->alias_type(field)->adr_type()

to compute the slice which is the slice for field in A. So the
slice used at parse time is the right one but during igvn, when the
slice is computed from the input address, a different slice (the one
for A + interface I) is used. That causes load/store nodes when
they are processed by igvn to use the wrong memory state.

In Compile::flatten_alias_type():

if (!ik->equals(canonical_holder) || tj->offset() != offset) {
  if( is_known_inst ) {
    tj = to = TypeInstPtr::make(to->ptr(), canonical_holder, true, nullptr, offset, to->instance_id());
  } else {
    tj = to = TypeInstPtr::make(to->ptr(), canonical_holder, false, nullptr, offset);
  }
}

only flattens the type if it's not the canonical holder but it should
test that the type doesn't implement interfaces that the canonical
holder doesn't. To keep the logic simple, the fix I propose creates a
new type whenever there's a chance that a type implements extra
interfaces (the type is not exact).

I also added asserts in GraphKit::make_load() and
GraphKit::store_to_memory() to make sure the slice that is passed
and the address type agree. Those asserts fire with the new test
cases. When running testing, I found that they also catch a few cases
in library_call.cpp where an incorrect slice is passed.

As further clean up, maybe we want to drop the slice argument to
GraphKit::make_load() and GraphKit::store_to_memory() (and to
their callers) given it's redundant with the address type and error
prone.

/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-8340214: C2 compilation asserts with "no node with a side effect" in PhaseIdealLoop::try_sink_out_of_loop (Bug - P2)

Reviewers

Christian Hagedorn (@chhagedorn - Reviewer)
Tobias Hartmann (@TobiHartmann - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21303

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-10-02T11:22:32Z

👋 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 · 2024-10-02T11:23:15Z

@rwestrel This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8340214: C2 compilation asserts with "no node with a side effect" in PhaseIdealLoop::try_sink_out_of_loop

Reviewed-by: chagedorn, thartmann

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 69 new commits pushed to the master branch:

20f36c6: 8339329: ConstantPoolBuilder#constantValueEntry method doc typo and clarifications
50426b3: 8337713: RISC-V: fix typos in macroAssembler_riscv.cpp
260d465: 8340572: ConcurrentModificationException when sorting ArrayList sublists
9a25f82: 8339386: Assertion on AIX - original PC must be in the main code section of the compiled method
df763cd: 8341558: [AIX] build broken after 8341413
1c3e56c: 8341512: Optimize StackMapGenerator::processInvokeInstructions
f8db3a8: 8341510: Optimize StackMapGenerator::processFieldInstructions
b42fbf4: 8339699: Optimize DataOutputStream writeUTF
5592894: 8340417: Open source some MenuBar tests - Set1
bade041: 8341554: Shenandoah: Missing heap lock when updating usage for soft ref policy
... and 59 more: https://git.openjdk.org/jdk/compare/855c8a7def21025bc2fc47594f7285a55924c213...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-10-02T11:23:53Z

@rwestrel
The hotspot-compiler label was successfully added.

mlbridge · 2024-10-02T11:26:46Z

Webrevs

chhagedorn

Looks reasonable.

This assert has proven to be quite valuable to find problems in the memory graph that we would otherwise miss. It was also one of the few assert that triggered when having a corrupted graph due to missing Assertion Predicates. I'm wondering if we need more such memory graph checks in general. Anyway, that's just a thought for some future RFE.

test/hotspot/jtreg/compiler/types/TestBadMemSliceWithInterfaces.java

src/hotspot/share/opto/compile.cpp

TobiHartmann

Looks reasonable to me. I submitted testing and will report back once it passed.

As further clean up, maybe we want to drop the slice argument to GraphKit::make_load() and GraphKit::store_to_memory() (and to their callers) given it's redundant with the address type and error prone.

Yes, let's do that. Please file a starter RFE.

TobiHartmann · 2024-10-02T11:35:01Z

src/hotspot/share/opto/compile.cpp

-        } else {
-          tj = to = TypeInstPtr::make(to->ptr(), canonical_holder, false, nullptr, offset);
-        }
+      assert(tj->offset() == offset, "not change to offset expected");


Suggested change

assert(tj->offset() == offset, "not change to offset expected");

assert(tj->offset() == offset, "no change to offset expected");

TobiHartmann · 2024-10-02T12:28:32Z

src/hotspot/share/opto/compile.cpp

+      if (xk && ik->equals(canonical_holder)) {
+        assert(tj == TypeInstPtr::make(to->ptr(), canonical_holder, is_known_inst, nullptr, offset, instance_id), "exact type should be canonical type");
+      } else {
+        assert(xk || !is_known_inst, "Known instance should be exact type");


Maybe add a comment here and explain the two cases when we create a new type.

Done in new commit.

…s.java Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

rwestrel · 2024-10-02T13:21:15Z

As further clean up, maybe we want to drop the slice argument to GraphKit::make_load() and GraphKit::store_to_memory() (and to their callers) given it's redundant with the address type and error prone.

Yes, let's do that. Please file a starter RFE.

https://bugs.openjdk.org/browse/JDK-8341411

TobiHartmann · 2024-10-02T13:31:43Z

src/hotspot/share/opto/compile.cpp

+      if (xk && ik->equals(canonical_holder)) {
+        assert(tj == TypeInstPtr::make(to->ptr(), canonical_holder, is_known_inst, nullptr, offset, instance_id), "exact type should be canonical type");
+      } else {
+        assert(xk || !is_known_inst, "Known instance should be exact type");


chhagedorn

Still good, one more minor thing.

test/hotspot/jtreg/compiler/types/TestBadMemSliceWithInterfaces.java

…s.java Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

rwestrel · 2024-10-09T07:16:10Z

@TobiHartmann Do you have an update on testing?

TobiHartmann · 2024-10-09T07:28:40Z

Sorry, that slipped through. Testing looked good. Let me re-run some quick testing with the latest updates.

TobiHartmann · 2024-10-09T10:33:57Z

All testing passed.

rwestrel · 2024-10-09T14:58:15Z

@TobiHartmann @chhagedorn thanks for the reviews.

rwestrel · 2024-10-09T14:58:20Z

/integrate

openjdk · 2024-10-09T14:59:17Z

Going to push as commit ff2f39f.
Since your change was applied there have been 109 commits pushed to the master branch:

ecc77a5: 8336702: C2 compilation fails with "all memory state should have been processed" assert
d936556: 8341633: StatSampler::assert_system_property: Print the keys and values of the assert
3fba170: 8340786: Introduce Predicate classes with predicate iterators and visitors for simplified walking
047c2d7: 8341141: Optimize DirectCodeBuilder
d636e0d: 8341688: Aarch64: Generate comments in -XX:+PrintInterpreter to link to source code
d3f3c6a: 8330157: C2: Add a stress flag for bailouts
d809bc0: 8341658: RISC-V: Test DateFormatProviderTest.java run timeouted
de90204: 8341588: Remove CollectionUsageThreshold.java from ProblemList-Xcomp for debugging
f276f58: 8341803: ProblemList containers/docker/TestJcmdWithSideCar.java on linux-x64
7eab0a5: 8337066: Repeated call of StringBuffer.reverse with double byte string returns wrong result
... and 99 more: https://git.openjdk.org/jdk/compare/855c8a7def21025bc2fc47594f7285a55924c213...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-09T14:59:25Z

@rwestrel Pushed as commit ff2f39f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

rwestrel added 2 commits October 2, 2024 10:19

fix & test

b7930c7

test cleanup

09f2e98

openjdk bot added rfr Pull request is ready for review hotspot-compiler hotspot-compiler-dev@openjdk.org labels Oct 2, 2024

chhagedorn approved these changes Oct 2, 2024

View reviewed changes

test/hotspot/jtreg/compiler/types/TestBadMemSliceWithInterfaces.java Outdated Show resolved Hide resolved

src/hotspot/share/opto/compile.cpp Outdated Show resolved Hide resolved

TobiHartmann approved these changes Oct 2, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 2, 2024

rwestrel and others added 3 commits October 2, 2024 14:56

Update test/hotspot/jtreg/compiler/types/TestBadMemSliceWithInterface…

c78f4af

…s.java Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

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

913a82b

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

Merge branch 'master' into JDK-8340214

ae7eb8f

openjdk bot removed the ready Pull request is ready to be integrated label Oct 2, 2024

review

46042b2

TobiHartmann approved these changes Oct 2, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 2, 2024

chhagedorn approved these changes Oct 2, 2024

View reviewed changes

test/hotspot/jtreg/compiler/types/TestBadMemSliceWithInterfaces.java Outdated Show resolved Hide resolved

Update test/hotspot/jtreg/compiler/types/TestBadMemSliceWithInterface…

6cdd233

…s.java Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

openjdk bot removed the ready Pull request is ready to be integrated label Oct 2, 2024

chhagedorn approved these changes Oct 3, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 3, 2024

TobiHartmann approved these changes Oct 7, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Oct 9, 2024

openjdk bot closed this Oct 9, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 9, 2024

graalvmbot mentioned this pull request Oct 14, 2024

[GR-58991] Update SyncPort info. oracle/graal#9873

Merged

theoweidmannoracle mentioned this pull request Nov 1, 2024

8341411: C2: remove slice parameter from GraphKit::make_load() and GraphKit::store_to_memory() #21834

Closed

3 tasks

	assert(tj->offset() == offset, "not change to offset expected");
	assert(tj->offset() == offset, "no change to offset expected");

8340214: C2 compilation asserts with "no node with a side effect" in PhaseIdealLoop::try_sink_out_of_loop #21303

8340214: C2 compilation asserts with "no node with a side effect" in PhaseIdealLoop::try_sink_out_of_loop #21303

Uh oh!

Conversation

rwestrel commented Oct 2, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Oct 2, 2024

Uh oh!

openjdk bot commented Oct 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Oct 2, 2024

Uh oh!

mlbridge bot commented Oct 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

chhagedorn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

TobiHartmann Oct 2, 2024

Choose a reason for hiding this comment

Uh oh!

TobiHartmann Oct 2, 2024

Choose a reason for hiding this comment

Uh oh!

rwestrel Oct 2, 2024

Choose a reason for hiding this comment

Uh oh!

TobiHartmann Oct 2, 2024

Choose a reason for hiding this comment

Uh oh!

rwestrel commented Oct 2, 2024

Uh oh!

TobiHartmann Oct 2, 2024

Choose a reason for hiding this comment

Uh oh!

chhagedorn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rwestrel commented Oct 9, 2024

Uh oh!

TobiHartmann commented Oct 9, 2024

Uh oh!

TobiHartmann commented Oct 9, 2024

Uh oh!

rwestrel commented Oct 9, 2024

Uh oh!

rwestrel commented Oct 9, 2024

Uh oh!

openjdk bot commented Oct 9, 2024

Uh oh!

openjdk bot commented Oct 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

rwestrel commented Oct 2, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 2, 2024 •

edited

Loading

mlbridge bot commented Oct 2, 2024 •

edited

Loading