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

gh-112779: Check 1-byte atomics in configure #112819

Merged
merged 3 commits into from
Dec 8, 2023

Conversation

colesbury
Copy link
Contributor

@colesbury colesbury commented Dec 6, 2023

This fixes RISC-V compilation for GCC versions earlier than GCC 13.

GCC versions 12 and earlier require libatomic library support for 1-byte and 2-byte atomics on RISC-V, but not for 8-byte atomics. The configure script only checked 8-byte atomics and so did not think that -latomic was necessary.

GCC 13 already worked. It does not require libatomic support for any of the sizes we use (1, 2, 4, or 8 bytes).

@colesbury
Copy link
Contributor Author

See https://gcc.godbolt.org/z/rPbrns6P8:

_Py_atomic_add_int8:
        addi    sp,sp,-16
        li      a2,5
        andi    a1,a1,0xff
        sd      ra,8(sp)
        call    __atomic_fetch_add_1
        ld      ra,8(sp)
        slliw   a0,a0,24
        sraiw   a0,a0,24
        addi    sp,sp,16
        jr      ra
_Py_atomic_add_int64:
        mv      a5,a0
        fence iorw,ow; amoadd.d.aq a0,a1,0(a5)
        ret

@colesbury colesbury marked this pull request as ready for review December 7, 2023 20:25
@corona10 corona10 self-assigned this Dec 8, 2023
@corona10
Copy link
Member

corona10 commented Dec 8, 2023

I will take a look at this weekend

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

Overall, it looks good to me.
But would you like to leave a comment why 1-byte atomic checking and 8-byte atomic is checking is needed with compiler version issue?

In the future, we need to know why the check-ing was needed but without comment, it could be easily forgotten.

@bedevere-app
Copy link

bedevere-app bot commented Dec 8, 2023

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@corona10 corona10 enabled auto-merge (squash) December 8, 2023 16:06
@corona10 corona10 merged commit 4d1eea5 into python:main Dec 8, 2023
31 of 32 checks passed
@colesbury colesbury deleted the riscv-atomics branch December 12, 2023 19:37
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.13 bugs and security fixes skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants