Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support opaque and acquire/release memory semantics #7517

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Spencer-Comin
Copy link
Contributor

@Spencer-Comin Spencer-Comin commented Oct 31, 2024

Expand the possible memory ordering semantics of symbols from volatile or non-volatile to volatile, acquire/release, opaque, or transparent. The memory ordering semantics are defined as follows:

Transparent

Only guaranteed to be bitwise atomic for data 32 bits or smaller and addresses.
This is the same as non-volatile semantics prior to this change.

Opaque

Accesses to opaque symbols are bitwise atomic.
The execution order of all opaque accesses to any given address in a single thread is the same as the program order of accesses to that address.

Acquire/Release

Loads of acquire/release symbols are acquire loads; i.e., loads and stores after a given acquire load will not be reordered to before that load. This matches the semantics of C's memory_order_acquire.
Stores to acquire/release symbols are release stores; i.e., loads and stores before a given release store wil not be reordered to after that store. This matches the semantics of C's memory_order_release.
Acquire/release accesses have a release-acquire ordering.
Acquire/release symbols also have all the same guarantees that opaque symbols have.

Volatile

Volatile accesses have a sequentially-consistent ordering. This matches the semantics of C's memory_order_seq_cst
Volatile symbols also have all the same guarantees that acquire/release symbols have.
This is the same as volatile semantics prior to this change

Additionally, see the notes on memory ordering semantics in the documentation for Java's VarHandle

@Spencer-Comin
Copy link
Contributor Author

OpenJ9 note: This requires a coordinated merge with eclipse-openj9/openj9#20475.

@Spencer-Comin Spencer-Comin force-pushed the ordered-opaque branch 4 times, most recently from e353977 to fead69c Compare November 7, 2024 15:24
@Spencer-Comin Spencer-Comin marked this pull request as ready for review November 7, 2024 15:42
@Spencer-Comin Spencer-Comin requested review from hzongaro and 0xdaryl and removed request for 0xdaryl November 7, 2024 15:44
@Spencer-Comin
Copy link
Contributor Author

Copy link
Contributor

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a review based on an initial pass through the changes. I'll come back for a more detailed review.

One high-level question I wanted to ask is whether these changes ought to be vetted at an OMR architecture meeting.

@Spencer-Comin Spencer-Comin force-pushed the ordered-opaque branch 4 times, most recently from 663a876 to 3a4d76a Compare November 26, 2024 19:10
@Spencer-Comin
Copy link
Contributor Author

Re @hzongaro's question

One high-level question I wanted to ask is whether these changes ought to be vetted at an OMR architecture meeting.

@0xdaryl @vijaysun-omr what do you think?

@vijaysun-omr
Copy link
Contributor

Jenkins build all

@vijaysun-omr
Copy link
Contributor

I will defer the question on whether we need architecture meeting review to Daryl, but I'll start running tests based on a review I just did.

@0xdaryl 0xdaryl self-assigned this Dec 6, 2024
Copy link
Contributor

@0xdaryl 0xdaryl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making these changes and thinking through the effects on each architecture.

My main concern is potential confusion around the terms used for the various memory orderings. "Opaque", for example, isn't a well-known term and I think is only used in Java circles. So seeing it appear throughout the code does take some mental adjustment if someone isn't already familiar with the Java definitions. Nevertheless, I am supportive of providing refinement to the kinds of memory ordering the compiler has to deal with, and we have to call them something.

In many places where isOpaque() appears, I think the semantics you're really trying to capture is !isTransparent(), because you're relying on the opaqueness property to be also true for acquire/release and volatile memory orderings. If that's the case, then (to me at least), it might be more readable to use that instead in those places.

@Spencer-Comin Spencer-Comin force-pushed the ordered-opaque branch 2 times, most recently from edf4efe to 0b6b3f1 Compare December 11, 2024 16:37
@Spencer-Comin
Copy link
Contributor Author

@0xdaryl @hzongaro @zl-wang could I get another round of review on this?

Copy link
Contributor

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the all updates that help to clarify the semantics!

I have some questions about whether some cases that are effectively testing for OpaqueSemantics or stronger ought to be testing for AcquireReleaseSemantics or stronger instead.

@Spencer-Comin
Copy link
Contributor Author

@hzongaro re: overly conservative treatment of OpaqueSemantics

Since being overly conservative is still correct (and equivalent to what we already have with volatile/plain), do you think it would be better to get these changes in as-is and then address relaxing the restrictions on individual optimizations in future PRs?

@hzongaro
Copy link
Contributor

re: overly conservative treatment of OpaqueSemantics

Since being overly conservative is still correct (and equivalent to what we already have with volatile/plain), do you think it would be better to get these changes in as-is and then address relaxing the restrictions on individual optimizations in future PRs?

Yes, that sounds reasonable. Getting initial support in for the different memory semantics will allow OMR and downstream projects to begin to take advantage of them, even if the treatment is relatively conservative today.

@hzongaro
Copy link
Contributor

hzongaro commented Feb 3, 2025

@Spencer-Comin, just so I'm clear, I believe the changes that you've made to address my review comments have all been to code that is in comments — 0b6b3f1..6080ed7 — and any further potential refinements might come in future pull requests. Is that correct? I just want to make sure I'm focusing on the correct set of recent changes.

@Spencer-Comin
Copy link
Contributor Author

@hzongaro yes that's correct

Copy link
Contributor

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the changes look good, but per the Commit Guidelines, may I ask you to reformat the text in the bodies of your commit comments so that no lines are longer than 72 characters?

This change expands the possible memory ordering semantics for a symbol
from volatile and non-volatile to volatile, acquire/release, opaque,
and transparent. An enum and helper methods are added to facilitate
working with memory ordering semantics.

Signed-off-by: Spencer Comin <[email protected]>
Copy link
Contributor

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all of your updates!

Copy link
Contributor

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry - late review question.

@0xdaryl
Copy link
Contributor

0xdaryl commented Feb 21, 2025

Jenkins build all

@0xdaryl
Copy link
Contributor

0xdaryl commented Feb 21, 2025

@Spencer-Comin : please see the two build failures on 32-bit ARM and zOS.

With the expansion of possible memory ordering semantics from binary
volatile or non-volatile to volatile, acquire/release, opaque, and
transparent, all tests whether a symbol is volatile need to be refined
depending on the intention of the test, i.e. is it testing if the
symbol is strictly volatile, simply mopaque, or somewhere in between?

Signed-off-by: Spencer Comin <[email protected]>
This change adds arrays for opaque and acquire/release unsafe symrefs
to the symbol reference table. Instead of having four separate fields,
the fields are combined into an array that can be indexed by the
OMR::Symbol::AccessMode enum.

Signed-off-by: Spencer Comin <[email protected]>
This flag is removed in OpenJ9.

Signed-off-by: Spencer Comin <[email protected]>
@0xdaryl
Copy link
Contributor

0xdaryl commented Feb 22, 2025

Jenkins build all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants