8343541: C1: Plain memory accesses are emitted with membars with +AlwaysAtomicAccesses

[pengxiaolong]

C1 and C2 has different implementations for +AlwaysAtomicAccesses, C2 impl only guarantees atomicity hence no membars are emitted for plain memory access, but C1 treats it same as volatile access hence it emits membars. The change removes the unnecessary membars in C1 for +AlwaysAtomicAccesses.

The test have been verified by very simple JMH benchmarks to measure the latency of reading/writing long/volatile long variable 10000 times, and run with VM option -XX:TieredStopAtLevel=3 -XX:+UnlockExperimentalVMOptions -XX:+AlwaysAtomicAccesses:

Before the fix:

Benchmark                                   Mode  Cnt       Score      Error  Units
AlwaysAtomicAccesses.testReadLong           avgt    5   58711.131 ±  716.940  ns/op
AlwaysAtomicAccesses.testReadVolatileLong   avgt    5   59014.735 ±  675.354  ns/op
AlwaysAtomicAccesses.testWriteLong          avgt    5  115817.978 ±  302.089  ns/op
AlwaysAtomicAccesses.testWriteVolatileLong  avgt    5  116317.835 ± 1451.365  ns/op

After the fix:

Benchmark                                   Mode  Cnt       Score      Error  Units
AlwaysAtomicAccesses.testReadLong           avgt    5   49651.527 ±  159.948  ns/op
AlwaysAtomicAccesses.testReadVolatileLong   avgt    5   58668.844 ±  316.029  ns/op
AlwaysAtomicAccesses.testWriteLong          avgt    5   23008.361 ±   10.947  ns/op
AlwaysAtomicAccesses.testWriteVolatileLong  avgt    5  116440.017 ± 1240.832  ns/op

---

Progress

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

Issue

• JDK-8343541: C1: Plain memory accesses are emitted with membars with +AlwaysAtomicAccesses (Enhancement - P4)

Reviewers

• Aleksey Shipilev (@shipilev - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22191

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back xpeng! 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]

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

[openjdk]

@pengxiaolong The following label will be automatically applied to this pull request:

• hotspot

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

[mlbridge]

Webrevs

• 01: Full - Incremental (df0fc908)
• 00: Full (8a532737)

[shipilev]

It is a bit sad that we call volatile_field_* when we really mean atomic. But renaming that would lead to much deeper changes. So I am okay with doing just this, thanks.

[pengxiaolong]

It is a bit sad that we call volatile_field_* when we really mean atomic. But renaming that would lead to much deeper changes. So I am okay with doing just this, thanks.

I'll fix it, it is not too deep, and they are only used by barrierSetC1.cpp

[shipilev]

It is a bit sad that we call volatile_field_* when we really mean atomic. But renaming that would lead to much deeper changes. So I am okay with doing just this, thanks.

I'll fix it, it is not too deep, and they are only used by barrierSetC1.cpp

No need. If we go down that road, we also want to rename lir_move_volatile and all that jazz, which would probably entail a few rounds of careful looking. Submit a follow-up issue for it instead.

[pengxiaolong]

It is a bit sad that we call volatile_field_* when we really mean atomic. But renaming that would lead to much deeper changes. So I am okay with doing just this, thanks.

I'll fix it, it is not too deep, and they are only used by barrierSetC1.cpp

You are right, checked the related code, need to update the chain deep to LIR_Assembler, I'll leave them unchanged except if it is really necessary.

[shipilev]

/reviewers 2

[openjdk]

@shipilev
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).