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

[Aleksey Shipilev]

On Mon, 18 Nov 2024 07:27:03 GMT, Xiaolong Peng <xpeng at openjdk.org> wrote:

> C1 and C2 has different implementations for `+AlwaysAtomicAccesses`, [C2 impl](https://github.com/openjdk/jdk/blob/4a7ce1d7c1bd4b751063b98cf8bedcd27055760b/src/hotspot/share/gc/shared/c2/barrierSetC2.cpp#L410) 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

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.

src/hotspot/share/gc/shared/c1/barrierSetC1.cpp line 144:

> 142:   DecoratorSet decorators = access.decorators();
> 143:   bool is_volatile = (decorators & MO_SEQ_CST) != 0;
> 144:   bool is_atomic_access = is_volatile || AlwaysAtomicAccesses;

Can be just `is_atomic`, I think.

-------------

PR Review: https://git.openjdk.org/jdk/pull/22191#pullrequestreview-2441950318
PR Review Comment: https://git.openjdk.org/jdk/pull/22191#discussion_r1846146160