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

[ATOMICSABI64]: Alpha Draft of Atomics ABI #256

Merged
merged 17 commits into from
Aug 30, 2024
Merged

Conversation

lukeg101
Copy link
Contributor

@lukeg101 lukeg101 commented Apr 5, 2024

atomicsabi64.pdf
This is the Alpha draft of the ABI for the
"C/C++ Atomics Application Binary Interface Standard for the Arm® 64-bit Architecture"

This document describes the C/C++ Atomics Application Binary Interface for the Arm 64-bit architecture.

This document concerns the valid mappings from C/C++ Atomic Operations to sequences of A64 instructions. This document does not support Armv7.

For matters concerning the memory model, please consult §B2 of the Arm Architecture Reference Manual.

We focus only on a subset of the C11 atomic operations and their mapping to A64 instructions at the time of writing.

More atomics will be added.

Co-Authored with Wilco Dijkstra (@Wilco1).

This is the Alpha draft of the ABI for the
"C/C++ Atomics Application Binary Interface Standard for the Arm® 64-bit Architecture"

This document describes the C/C++ Atomics Application Binary Interface for the Arm 64-bit architecture.

This document concerns the valid mappings from C/C++ Atomic Operations to sequences of A64 instructions.
This document does not support Armv7.

For matters concerning the memory model, please consult §B2 of the Arm Architecture Reference Manual.

We focus only on a subset of the C11 atomic operations and their mapping to A64 instructions at the time of writing.

More atomics will be added.

Co-Authored with Wilco Dijkstra (@Wilco1).
@lukeg101
Copy link
Contributor Author

lukeg101 commented Apr 5, 2024

@stuij @smithp35 @Wilco1

Copy link
Contributor

@smithp35 smithp35 left a comment

Choose a reason for hiding this comment

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

Thanks very much for putting this together. Most of my comments are based on first time reading rather than as a concurrency expert.

atomicsabi64/atomicsabi64.rst Show resolved Hide resolved
atomicsabi64/atomicsabi64.rst Outdated Show resolved Hide resolved
atomicsabi64/atomicsabi64.rst Outdated Show resolved Hide resolved
atomicsabi64/README.md Outdated Show resolved Hide resolved
atomicsabi64/atomicsabi64.rst Outdated Show resolved Hide resolved
atomicsabi64/atomicsabi64.rst Outdated Show resolved Hide resolved
atomicsabi64/atomicsabi64.rst Outdated Show resolved Hide resolved
atomicsabi64/atomicsabi64.rst Outdated Show resolved Hide resolved
atomicsabi64/atomicsabi64.rst Outdated Show resolved Hide resolved
atomicsabi64/atomicsabi64.rst Show resolved Hide resolved
atomicsabi64/atomicsabi64.rst Outdated Show resolved Hide resolved
atomicsabi64/atomicsabi64.rst Outdated Show resolved Hide resolved
Copy link

@walhua walhua left a comment

Choose a reason for hiding this comment

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

LGTM, just added some trivial comments.

atomicsabi64/atomicsabi64.rst Outdated Show resolved Hide resolved
atomicsabi64/atomicsabi64.rst Outdated Show resolved Hide resolved
atomicsabi64/atomicsabi64.rst Outdated Show resolved Hide resolved
atomicsabi64/atomicsabi64.rst Outdated Show resolved Hide resolved
atomicsabi64/atomicsabi64.rst Outdated Show resolved Hide resolved
atomicsabi64/atomicsabi64.rst Outdated Show resolved Hide resolved
atomicsabi64/atomicsabi64.rst Outdated Show resolved Hide resolved
atomicsabi64/atomicsabi64.rst Outdated Show resolved Hide resolved
atomicsabi64/atomicsabi64.rst Outdated Show resolved Hide resolved
@lukeg101
Copy link
Contributor Author

lukeg101 commented May 2, 2024

Apologies for my delay folks, I think I've addressed everything here now with the new version
atomicsabi64.pdf

On the footnote front, I had to use a * in a table above the main table of mappings, since the default [1]_ syntax moves the footnote all the way to the bottom of the document itself.

Copy link
Contributor

@smithp35 smithp35 left a comment

Choose a reason for hiding this comment

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

Thanks very much for splitting out the specification and the rationale. I've given both a read through and have some mostly cosmetic comments.

Operations include Thread Fences.

Shared-Memory Location
A memory location that can be accessed by any Thread of Execution in the
Copy link
Contributor

Choose a reason for hiding this comment

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

By any Thread of Execution do you mean any one thread or all threads?

Intuitively I'd expect shared-memory location to be accessible by more than one Thread of Execution, but not necessarily all.

If I'm write perhaps: "by more than one Thread of Execution"

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed, I've changed it to Thread and simplified definitions.

following sub-components.

* The `Mappings from Atomic Operations to Assembly Sequences`_, which defines
the Mappings from C/C++ atomic operations to sto one of more Assembly
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo? to sto

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed

atomicsabi64/atomicsabi64.rst Show resolved Hide resolved
atomicsabi64/atomicsabi64.rst Show resolved Hide resolved
+-------------------------------------------------------------------------------------------+
| Note |
+===========================================================================================+
| ``*`` Using ``WZR`` or ``XZR`` for the destination register is invalid (Section 4.7). |
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming this * corresponds to the * on some of the sequences in the table?

I think it would be be good to make this explicit. For example
In the tables below, in sequences with * Using WZR ...

Alternatively we could do something like note1 or a footnote instead of *.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed, added before table rather than as a note at end. Normal footnotes didn't work properly, placed at very end of document...

design-documents/atomics-ABI.rst Outdated Show resolved Hide resolved
design-documents/atomics-ABI.rst Outdated Show resolved Hide resolved
design-documents/atomics-ABI.rst Outdated Show resolved Hide resolved
design-documents/atomics-ABI.rst Outdated Show resolved Hide resolved
design-documents/atomics-ABI.rst Show resolved Hide resolved
@stuij stuij requested a review from sallyarmneale August 12, 2024 12:59
atomicsabi64/atomicsabi64.rst Outdated Show resolved Hide resolved
atomicsabi64/atomicsabi64.rst Outdated Show resolved Hide resolved
atomicsabi64/atomicsabi64.rst Outdated Show resolved Hide resolved
atomicsabi64/atomicsabi64.rst Outdated Show resolved Hide resolved
atomicsabi64/atomicsabi64.rst Outdated Show resolved Hide resolved
design-documents/atomics-ABI.rst Outdated Show resolved Hide resolved
design-documents/atomics-ABI.rst Outdated Show resolved Hide resolved
design-documents/atomics-ABI.rst Outdated Show resolved Hide resolved
design-documents/atomics-ABI.rst Outdated Show resolved Hide resolved
design-documents/atomics-ABI.rst Outdated Show resolved Hide resolved
design-documents/atomics-ABI.rst Show resolved Hide resolved
design-documents/atomics-ABI.rst Outdated Show resolved Hide resolved
design-documents/atomics-ABI.rst Outdated Show resolved Hide resolved
design-documents/atomics-ABI.rst Outdated Show resolved Hide resolved
design-documents/atomics-ABI.rst Outdated Show resolved Hide resolved
The content of this specification is a draft, and Arm considers the
likelihood of future incompatible changes to be significant.

All content in this document is at the **Release** quality level.
Copy link
Member

Choose a reason for hiding this comment

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

This should be **Alpha** quality I believe.

atomicsabi64/atomicsabi64.rst Outdated Show resolved Hide resolved
design-documents/atomics-ABI.rst Outdated Show resolved Hide resolved
design-documents/atomics-ABI.rst Outdated Show resolved Hide resolved
design-documents/atomics-ABI.rst Outdated Show resolved Hide resolved
design-documents/atomics-ABI.rst Outdated Show resolved Hide resolved
design-documents/atomics-ABI.rst Show resolved Hide resolved
Copy link
Member

@stuij stuij left a comment

Choose a reason for hiding this comment

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

Thanks Luke and Wilco for your patience with the barrage of comments. Looks like all the reviewers are happy with the current state of the docs right now.

If there are no objections, I'll push this tomorrow.

Copy link
Contributor

@smithp35 smithp35 left a comment

Choose a reason for hiding this comment

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

LGTM thanks very much for all the hard work.

@stuij stuij merged commit 3ec80bd into ARM-software:main Aug 30, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants