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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-108337: Add pyatomic.h header #108701

Merged
merged 1 commit into from
Aug 31, 2023
Merged

gh-108337: Add pyatomic.h header #108701

merged 1 commit into from
Aug 31, 2023

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Aug 30, 2023

This adds a new header that provides atomic operations on common data types. The intention is that this will be exposed through Python.h, although that is not the case yet. The only immediate use is in the test file.


馃摎 Documentation preview 馃摎: https://cpython-previews--108701.org.readthedocs.build/

@vstinner
Copy link
Member Author

This PR is a copy of PR #108338 with my changes:

  • Many coding style changes

  • Add lines like // --- _Py_atomic_add -------------------------------------------------------- for readability

  • Rewrite pyatomic_msc.h to minimize functons using the Windows API, most functions now reuse _Py_atomic functions. For example, _Py_atomic_add_uint8() just calls _Py_atomic_add_uint8().

  • pyatomic.h:

    • At the top, list all functions but a "pseudo Python code" implementation.
    • Undefine macros only defined to include pyatomic_xxx.h headers.
  • pyatomic_msc.h:

    • add _Py_atomic_ASSERT_ARG_TYPE() macro (build assertion)
    • reuse "load relaxed" functons in functions implementations as loops, like _Py_atomic_add_int64() calls _Py_atomic_load_int64_relaxed(). It reduces the number of lines of code using the volatile keyword.
  • pyatomic_gcc.h more compact static inline body on a single line for most functions.

  • pyatomic_std.h:

    • _Py_USING_STD;: add ; semi-colon. It prevents code formatter to want to change the indentation of the following line.
    • Indent the #include and #define at the top of the line (add two spaces),for readability.

@vstinner
Copy link
Member Author

cc @colesbury

@corona10
Copy link
Member

corona10 commented Aug 31, 2023

Out of curiosity, https://clang.llvm.org/docs/LanguageExtensions.html#langext-c11-atomic
No clang header(pyatomic_clang.h) is needed?

@colesbury
Copy link
Contributor

@corona10, Clang supports the GCC atomic builtins.

@vstinner
Copy link
Member Author

Out of curiosity, https://clang.llvm.org/docs/LanguageExtensions.html#langext-c11-atomic

Are there advantages on using these clang functions, instead of using the pyatomic_gcc.h on clang?

@colesbury
Copy link
Contributor

@vstinner, no, both sets of intrinsics work fine on Clang

Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

Thanks @vstinner, this looks good to me

@vstinner
Copy link
Member Author

PR rebased on the main branch to fix the merge conflict.

This adds a new header that provides atomic operations on common data
types. The intention is that this will be exposed through Python.h,
although that is not the case yet. The only immediate use is in
the test file.
@vstinner vstinner enabled auto-merge (squash) August 31, 2023 21:11
@vstinner vstinner merged commit 2bd960b into python:main Aug 31, 2023
22 checks passed
@vstinner vstinner deleted the pyatomic branch August 31, 2023 21:41
@vstinner
Copy link
Member Author

vstinner commented Aug 31, 2023

The final commit look like this:

commit 2bd960b57944107fbfbd8ff005b4223e1ea6555f
Author: Victor Stinner <vstinner@python.org>
Date:   Thu Aug 31 23:41:18 2023 +0200

    gh-108337: Add pyatomic.h header (#108701)
    
    This adds a new header that provides atomic operations on common data
    types. The intention is that this will be exposed through Python.h,
    although that is not the case yet. The only immediate use is in
    the test file.
    
    Co-authored-by: Sam Gross <colesbury@gmail.com>

Locally, Sam Gross was the author. I don't understand how GitHub merge works. It seems to change the author, sorry about that :-( I'm not used to "copy" a PR to propose some changes.

@vstinner
Copy link
Member Author

Thanks @colesbury, that's a nice addition! It looks better than Include/internal/pycore_atomic_funcs.h and Include/internal/pycore_atomic.h.

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.

None yet

4 participants