8379260: C2: Separate volatile barrier and full barrier by merykitty · Pull Request #30106 · openjdk/jdk

merykitty · 2026-03-06T02:58:55Z

Hi,

MemBarVolatileNode is described as:

// Ordering between a volatile store and a following volatile load.
// Requires multi-CPU visibility?
class MemBarVolatileNode: public MemBarNode

This is incorrect, as MemBarVolatileNode is used in VarHandle::fullFence intrinsics, which means it must act as a full fence. In addition, since MemBarVolatileNode must act as a full fence, it prevents most optimizations with volatile accesses.

This PR extracts MemBarFull out of MemBarVolatile as a proper full fence. This removes the confusion in the description of MemBarVolatileNode, as well as allows us to have a better chance optimizing memory accesses around volatile accesses.

Testing:

tier1,tier2,tier3,tier4,hs-comp-stress

Please kindly review, thanks a lot.

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-8379260: C2: Separate volatile barrier and full barrier (Bug - P4)

Reviewers

Andrew Haley (@theRealAph - Reviewer) Review applies to 6756bcdf
Dean Long (@dean-long - Reviewer) Review applies to 6756bcdf
Fei Yang (@RealFYang - Reviewer)
Martin Doerr (@TheRealMDoerr - Reviewer)
Amit Kumar (@offamitkumar - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 30106

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2026-03-06T03:00:05Z

👋 Welcome back qamai! 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 · 2026-03-06T03:01:22Z

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

8379260: C2: Separate volatile barrier and full barrier

Reviewed-by: fyang, mdoerr, amitkumar, aph, dlong

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

39a2566: 8378740: Suppressed warnings reported when implicit compilation is combined with annotation processing
814b7f5: 8378920: Remove AppContext from SequencedEvent
30e569d: 8379021: Shenandoah: Speedup ShenandoahSimpleBitMapTest
... and 109 more: https://git.openjdk.org/jdk/compare/c87ecaddfb32485b2ecaecc9284ce37b610ffffc...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 · 2026-03-06T03:02:02Z

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

hotspot-compiler

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 · 2026-03-06T03:05:43Z

Webrevs

dean-long · 2026-03-06T08:37:37Z

  access_store_at(nullptr, jt_addr, _gvn.type(jt_addr)->is_ptr(), ideal.ConI(1), TypeInt::BOOL, T_BOOLEAN, IN_NATIVE | MO_UNORDERED);
  access_store_at(nullptr, vt_addr, _gvn.type(vt_addr)->is_ptr(), ideal.ConI(1), TypeInt::BOOL, T_BOOLEAN, IN_NATIVE | MO_UNORDERED);
-  insert_mem_bar(Op_MemBarVolatile);
+  insert_mem_bar(Op_MemBarFull);


The comment says this code needs OrderAccess::storeload(), which can be weaker than a full fence on PPC and RISC-V. Should we also introduce a new Op_MemBarStoreLoad to support this?

That's a valid point, I have updated the PR.

merykitty · 2026-03-06T09:41:19Z

I see that on RISC-V, MemBarVolatile is implemented as __ membar(MacroAssembler::StoreLoad), but MacroAssembler::StoreLoad is different from MacroAssembler::AnyAny, can anyone help me understand if I should raise this to a bug?

dean-long · 2026-03-06T12:27:25Z

MemBarVolatile may be obsolete now if we have MemBarStoreLoad. My understanding is that MemBarVolatile is needed between a volatile store and a volatile load, and the old JSR-133 cookbook says that requires only MacroAssembler::StoreLoad, not the stronger MacroAssembler::AnyAny.

merykitty · 2026-03-06T12:40:31Z

@dean-long A MemBarVolatile is weaker than a MemBarStoreLoad. A MemBarVolatile only prevents volatile accesses from moving past it, while a MemBarStoreLoad prevents all accesses from moving past it.

merykitty · 2026-03-06T12:48:43Z

My understanding is that MemBarVolatile is needed between a volatile store and a volatile load, and the old JSR-133 cookbook says that requires only MacroAssembler::StoreLoad, not the stronger MacroAssembler::AnyAny.

But MemBarVolatile is used to implement VarHandle::fullFence, so it must act as a full-fence, a store-load is simply inadequate.

theRealAph · 2026-03-06T13:30:59Z

@dean-long A MemBarVolatile is weaker than a MemBarStoreLoad. A MemBarVolatile only prevents volatile accesses from moving past it, while a MemBarStoreLoad prevents all accesses from moving past it.

That's not right.

In addition to obeying Acquire and Release properties, all Volatile operations are totally ordered with respect to each other. So, a MemBarVolatile is release; acquire; plus (in effect) a storeLoad sequenced before the next volatile load.

theRealAph · 2026-03-06T13:32:03Z

I see that on RISC-V, MemBarVolatile is implemented as __ membar(MacroAssembler::StoreLoad), but MacroAssembler::StoreLoad is different from MacroAssembler::AnyAny, can anyone help me understand if I should raise this to a bug?

I suppose it is a bug, yes.

merykitty · 2026-03-06T13:43:31Z

In addition to obeying Acquire and Release properties, all Volatile operations are totally ordered with respect to each other. So, a MemBarVolatile is release; acquire; plus (in effect) a storeLoad sequenced before the next volatile load.

A volatile load is a load followed by a MemBarAcquire. A volatile store is a MemBarRelease followed by a store. So, MemBarVolatile only has the responsibility of ensuring a volatile store is not reordered with subsequent volatile loads. When support_IRIW_for_not_multiple_copy_atomic_cpu is true, the MemBarVolatile is inserted before the volatile loads, while if it is false, the barrier is inserted after the volatile stores.

I suppose it is a bug, yes.

Thanks, I changed it to a bug.

theRealAph · 2026-03-06T15:49:48Z

In addition to obeying Acquire and Release properties, all Volatile operations are totally ordered with respect to each other. So, a MemBarVolatile is release; acquire; plus (in effect) a storeLoad sequenced before the next volatile load.

A volatile load is a load followed by a MemBarAcquire. A volatile store is a MemBarRelease followed by a store.

So, MemBarVolatile only has the responsibility of ensuring a volatile store is not reordered with subsequent volatile loads.

Oh, ISWYM. MemBarVolatile, despite its name, is not a volatile fence. It is C2's name for a StoreLoad needed between volatile stores and loads. In practice that usually means a full fence, but not always.

theRealAph · 2026-03-06T15:55:08Z

+  format %{ "membar_full" %}
+  ins_encode %{
+    __ block_comment("membar_full");
+    __ membar(Assembler::AnyAny);


This code is making a distinction without a real difference between StoreLoad and AnyAny: it's dmb ish in both cases.

Yes, that's right, so there is no difference functional-wise on Aarch64. If you want to ask me why having a separate match block for MemBarFull and MemBarStoreLoad, I think the difference in the comment is useful.

theRealAph · 2026-03-06T16:13:32Z

Oh, ISWYM. MemBarVolatile, despite its name, is not a volatile fence. It is C2's name for a StoreLoad needed between volatile stores and loads. In practice that usually means a full fence, but not always.

So, what is MemBarVolatile for, when it's just a StoreLoad?

Co-authored-by: Andrew Haley <aph-open@littlepinkcloud.com>

merykitty · 2026-03-06T16:15:56Z

A MemBarVolatile is weaker than a MemBarStoreLoad, so we can take advantage of that to optimize non-volatile accesses around it in the future.

dean-long · 2026-03-06T23:39:57Z

But MemBarVolatile is used to implement VarHandle::fullFence, so it must act as a full-fence, a store-load is simply inadequate.

Yes, VarHandle::fullFence was using MemBarVolatile, but it should be using MemBarFull now.

A MemBarVolatile is weaker than a MemBarStoreLoad, so we can take advantage of that to optimize non-volatile accesses around it in the future.

Both are really StoreLoad now, if we consitently use MemBarFull when a full fence is needed, and MemBarStoreLoad only when a StoreLoad is needed. StoreLoad != full fence even though they both might get translated to the same thing depending on the hardware.

merykitty · 2026-03-07T02:26:37Z

MemBarStoreLoad and MemBarVolatile are the same at code generation, but the compiler can move non-volatile accesses past MemBarVolatile when it is not normally possible with MemBarStoreLoad.

For example, if we have a volatile store in a loop and no other store or memory barrier in that loop, a MemBarStoreLoad will prevent all loads from being hoisted out. This is even a little inconsistent, because we only emit that barrier after the volatile store if support_IRIW_for_not_multiple_copy_atomic_cpu is false. Otherwise, the barrier is emitted before the volatile loads, so the loads can be hoisted fine.

dean-long · 2026-03-07T08:14:35Z

Thanks, I hope I got it now :-). MemBarVolatile semantics are more at the Java level, while MemBarStoreLoad is closer to the memory level, I guess you could say?

theRealAph · 2026-03-07T10:17:58Z

MemBarStoreLoad and MemBarVolatile are the same at code generation, but the compiler can move non-volatile accesses past MemBarVolatile when it is not normally possible with MemBarStoreLoad.

For example, if we have a volatile store in a loop and no other store or memory barrier in that loop, a MemBarStoreLoad will prevent all loads from being hoisted out. This is even a little inconsistent, because we only emit that barrier after the volatile store if support_IRIW_for_not_multiple_copy_atomic_cpu is false. Otherwise, the barrier is emitted before the volatile loads, so the loads can be hoisted fine.

That makes sense, so a MemBarVolatile can float around betwen seqCst stores and seqCst loads.

theRealAph

Looks good.

RealFYang

Hi, I have two minor comments about the instruct format for RISC-V.

RealFYang · 2026-03-10T01:19:12Z

+  ins_cost(VOLATILE_REF_COST);
+
+  format %{ "#@membar_full_rvtso\n\t"
+            "fence a, a"%}


Suggestion: "fence rw, rw"%}

Thanks a lot for your review, I have made the change.

RealFYang · 2026-03-10T01:21:44Z

+  ins_cost(VOLATILE_REF_COST);
+
+  format %{ "#@membar_full_rvwmo\n\t"
+            "fence a, a"%}


Suggestion: "fence rw, rw"%}

RealFYang

Thanks for the update. The RISC-V part looks good.

merykitty · 2026-03-10T07:12:54Z

@theRealAph @dean-long @RealFYang Thanks a lot for your reviews and suggestions. Can I integrate now or should I wait for approval for the ppc and s390 parts?

dean-long · 2026-03-11T08:51:18Z

Yes, wait for ppc and s390 and maybe ping the port maintainers.

merykitty · 2026-03-11T09:31:10Z

@TheRealMDoerr Could you take a look? Thanks in advance.

TheRealMDoerr

LGTM. Also tested jtreg:test/hotspot/jtreg/compiler/c2 on linux ppc64le. Thanks!

merykitty · 2026-03-11T12:51:22Z

@offamitkumar Could you review the s390 part, please?

offamitkumar

s390 looks fine, tier1 tests are also clean.

merykitty · 2026-03-12T08:37:46Z

Thanks very much for your reviews!

/integrate

openjdk · 2026-03-12T08:39:42Z

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

73424d1: 8379506: Parallel: Move Parallel specific flags to parallel_globals.hpp
26bb357: 8305250: Unnecessary "unknown enum constant" warning emitted by javac when dependency has optional annotations with enums
39a2566: 8378740: Suppressed warnings reported when implicit compilation is combined with annotation processing
... and 111 more: https://git.openjdk.org/jdk/compare/c87ecaddfb32485b2ecaecc9284ce37b610ffffc...master

Your commit was automatically rebased without conflicts.

openjdk · 2026-03-12T08:39:53Z

@merykitty Pushed as commit fd80329.

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

Separate MemBarVolatile and MemBarFull

63d73c3

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Mar 6, 2026

openjdk bot added the rfr Pull request is ready for review label Mar 6, 2026

dean-long reviewed Mar 6, 2026

View reviewed changes

MemBarStoreLoad

405c5d1

theRealAph reviewed Mar 6, 2026

View reviewed changes

Comment thread src/hotspot/cpu/aarch64/aarch64.ad Outdated

Update format of membar_full in aarch64

6756bcd

Co-authored-by: Andrew Haley <aph-open@littlepinkcloud.com>

theRealAph approved these changes Mar 9, 2026

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 9, 2026

dean-long approved these changes Mar 9, 2026

View reviewed changes

RealFYang reviewed Mar 10, 2026

View reviewed changes

RISC-V change a to rw, aarch64 remove trailing whitespace

76b864c

openjdk bot removed the ready Pull request is ready to be integrated label Mar 10, 2026

RealFYang approved these changes Mar 10, 2026

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 10, 2026

TheRealMDoerr approved these changes Mar 11, 2026

View reviewed changes

offamitkumar approved these changes Mar 12, 2026

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Mar 12, 2026

openjdk bot closed this Mar 12, 2026

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 12, 2026

Conversation

merykitty commented Mar 6, 2026 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Mar 6, 2026

Uh oh!

openjdk bot commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dean-long Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

merykitty Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

merykitty commented Mar 6, 2026

Uh oh!

dean-long commented Mar 6, 2026

Uh oh!

merykitty commented Mar 6, 2026

Uh oh!

merykitty commented Mar 6, 2026

Uh oh!

theRealAph commented Mar 6, 2026

Uh oh!

theRealAph commented Mar 6, 2026

Uh oh!

merykitty commented Mar 6, 2026

Uh oh!

theRealAph commented Mar 6, 2026

Uh oh!

theRealAph Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

merykitty Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

theRealAph commented Mar 6, 2026

Uh oh!

merykitty commented Mar 6, 2026

Uh oh!

dean-long commented Mar 6, 2026

Uh oh!

merykitty commented Mar 7, 2026

Uh oh!

dean-long commented Mar 7, 2026

Uh oh!

theRealAph commented Mar 7, 2026

Uh oh!

theRealAph left a comment

Choose a reason for hiding this comment

Uh oh!

RealFYang left a comment

Choose a reason for hiding this comment

Uh oh!

RealFYang Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

merykitty Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

RealFYang Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

RealFYang left a comment

Choose a reason for hiding this comment

Uh oh!

merykitty commented Mar 10, 2026

merykitty commented Mar 6, 2026 •

edited by openjdk bot

Loading

openjdk bot commented Mar 6, 2026 •

edited

Loading

openjdk bot commented Mar 6, 2026 •

edited

Loading

mlbridge bot commented Mar 6, 2026 •

edited

Loading