8320807: [PPC64][ZGC] C1 generates wrong code for atomics by TheRealMDoerr · Pull Request #16835 · openjdk/jdk

TheRealMDoerr · 2023-11-27T21:52:53Z

Debugging test failures on PPC64 in java/lang/Thread/virtual/stress/Skynet.java#ZGenerational has shown that the ldarx+stdcx_ loop for uncompressed Oops in LIR_Assembler::atomic_op is wrong: __ mr(Rtmp, Robj); is inside of the ldarx+stdcx_ loop, but must be outside of it. Repeated execution leads to wrong store value.
In addition, zBarrierSetC1.cpp expects cas_obj and xchg to contain all necessary memory barriers. That doesn't fit to the current PPC64 design which inserts memory barriers on LIR level instead. I've changed this and moved them into the assembler code for all GCs.
While debugging, I have optimized out an unnecessary branch in ZBarrierSetAssembler::store_barrier_medium.

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-8320807: [PPC64][ZGC] C1 generates wrong code for atomics (Bug - P3)

Reviewers

Lutz Schmidt (@RealLucy - Reviewer) ⚠️ Review applies to 54a0d25d
Richard Reingruber (@reinrich - Reviewer) ⚠️ Review applies to 54a0d25d

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16835

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-11-27T21:53:50Z

👋 Welcome back mdoerr! 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 · 2023-11-27T21:54:32Z

@TheRealMDoerr The following labels will be automatically applied to this pull request:

hotspot
shenandoah

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-11-27T21:59:27Z

Webrevs

01: Full - Incremental (e6958e4c)
00: Full (54a0d25d)

RealLucy

Looks good.
Thanks for detecting and fixing this bug.

openjdk · 2023-12-01T11:08:27Z

@TheRealMDoerr 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:

8320807: [PPC64][ZGC] C1 generates wrong code for atomics

Reviewed-by: lucy, rrich

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 90 new commits pushed to the master branch:

3b30095: 8321130: Microbenchmarks do not build any more after 8254693 on 32 bit platforms
8f1d40b: 8321114: Rename "Unnamed Classes" to "Implicitly Declared Classes" better
ecd335d: 8321107: Add more test cases for JDK-8319372
2476be4: 8320716: ResolvedModule::reads includes self when configuration contains two or more automatic modules
4ba94ef: 8320145: Compiler should accept final variable in Record Pattern
02ffab1: 8321141: VM build issue on MacOS after JDK-8267532
56d4c33: 8227529: With malformed --app-image the error messages are awful
76fea80: 8320608: Many jtreg printing tests are missing the @printer keyword
c4732c2: 8320530: has_resolved_ref_index flag not restored after resetting entry
e96e191: 8318586: Explicitly handle upcall stub allocation failure
... and 80 more: https://git.openjdk.org/jdk/compare/6871a2ff1207d3ee70973b1c4ee9bd09969c185b...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.

reinrich

Looks good. Just two suggestions to improve comments.
Cheers, Richard.

reinrich · 2023-12-01T11:26:18Z

src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp

    Unimplemented();
  }

+  // Volatile load may be followed by Unsafe CAS.


Suggested change

// Volatile load may be followed by Unsafe CAS.

// There might be a volatile load before this Unsafe CAS.

reinrich · 2023-12-01T11:31:20Z

src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp

+    }
+  }
+
+  // Volatile load may be followed by Unsafe OP.


Suggested change

// Volatile load may be followed by Unsafe OP.

// There might be a volatile load before this Unsafe OP.

TheRealMDoerr · 2023-12-01T13:18:26Z

Thanks for the reviews! I've improved the comments as suggested.

TheRealMDoerr · 2023-12-01T14:43:39Z

/integrate

openjdk · 2023-12-01T14:44:23Z

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

54957ac: 8312560: Annotation on Decomposed Record Component in Enhanced For Loop Fails Compilation
3b30095: 8321130: Microbenchmarks do not build any more after 8254693 on 32 bit platforms
8f1d40b: 8321114: Rename "Unnamed Classes" to "Implicitly Declared Classes" better
ecd335d: 8321107: Add more test cases for JDK-8319372
2476be4: 8320716: ResolvedModule::reads includes self when configuration contains two or more automatic modules
4ba94ef: 8320145: Compiler should accept final variable in Record Pattern
02ffab1: 8321141: VM build issue on MacOS after JDK-8267532
56d4c33: 8227529: With malformed --app-image the error messages are awful
76fea80: 8320608: Many jtreg printing tests are missing the @printer keyword
c4732c2: 8320530: has_resolved_ref_index flag not restored after resetting entry
... and 81 more: https://git.openjdk.org/jdk/compare/6871a2ff1207d3ee70973b1c4ee9bd09969c185b...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-12-01T14:44:30Z

@TheRealMDoerr Pushed as commit 3087e14.

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

[PPC64][ZGC] C1 generates wrong code for atomics

54a0d25

openjdk bot added hotspot hotspot-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Nov 27, 2023

TheRealMDoerr changed the title ~~[PPC64][ZGC] C1 generates wrong code for atomics~~ 8320807: [PPC64][ZGC] C1 generates wrong code for atomics Nov 27, 2023

openjdk bot added the rfr Pull request is ready for review label Nov 27, 2023

RealLucy approved these changes Dec 1, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 1, 2023

reinrich approved these changes Dec 1, 2023

View reviewed changes

Improve comments.

e6958e4

openjdk bot added the integrated Pull request has been integrated label Dec 1, 2023

openjdk bot closed this Dec 1, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 1, 2023

TheRealMDoerr deleted the 8320807_PPC64_ZGC branch December 1, 2023 14:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8320807: [PPC64][ZGC] C1 generates wrong code for atomics#16835

8320807: [PPC64][ZGC] C1 generates wrong code for atomics#16835
TheRealMDoerr wants to merge 2 commits intoopenjdk:masterfrom
TheRealMDoerr:8320807_PPC64_ZGC

TheRealMDoerr commented Nov 27, 2023 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented Nov 27, 2023

Uh oh!

openjdk bot commented Nov 27, 2023

Uh oh!

mlbridge bot commented Nov 27, 2023 •

edited

Loading

Uh oh!

RealLucy left a comment

Uh oh!

openjdk bot commented Dec 1, 2023 •

edited

Loading

Uh oh!

reinrich left a comment

Uh oh!

reinrich Dec 1, 2023

Uh oh!

reinrich Dec 1, 2023

Uh oh!

TheRealMDoerr commented Dec 1, 2023

Uh oh!

TheRealMDoerr commented Dec 1, 2023

Uh oh!

openjdk bot commented Dec 1, 2023

Uh oh!

openjdk bot commented Dec 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

	// Volatile load may be followed by Unsafe CAS.
	// There might be a volatile load before this Unsafe CAS.

Conversation

TheRealMDoerr commented Nov 27, 2023 • 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 Nov 27, 2023

Uh oh!

openjdk bot commented Nov 27, 2023

Uh oh!

mlbridge bot commented Nov 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

RealLucy left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Dec 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reinrich left a comment

Choose a reason for hiding this comment

Uh oh!

reinrich Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

reinrich Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

TheRealMDoerr commented Dec 1, 2023

Uh oh!

TheRealMDoerr commented Dec 1, 2023

Uh oh!

openjdk bot commented Dec 1, 2023

Uh oh!

openjdk bot commented Dec 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

TheRealMDoerr commented Nov 27, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 27, 2023 •

edited

Loading

openjdk bot commented Dec 1, 2023 •

edited

Loading