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

be/c: replace __atomic-builtins by c11 atomics #2563

Merged
merged 1 commit into from
Feb 22, 2024

Conversation

michaellilltokiwa
Copy link
Member

@michaellilltokiwa
Copy link
Member Author

@fridis @maxteufel I need some guidance here. I'm all but certain that C11-atomics are preferable to the builtins.
We could keep both implementations defaulting to C11?

Microsoft added support in their compiler for C11-atomics at end of 2022:
https://devblogs.microsoft.com/cppblog/c11-atomics-in-visual-studio-2022-version-17-5-preview-2/

@maxteufel
Copy link
Collaborator

After pondering a while, I think if GCC, Clang, and MSVC support C11-style atomics, then we should use those. Why shouldn't we use the standard over custom implementations?

@michaellilltokiwa michaellilltokiwa marked this pull request as ready for review February 20, 2024 14:00
@michaellilltokiwa michaellilltokiwa added the C backend related to the C backend label Feb 21, 2024
Copy link
Member

@fridis fridis left a comment

Choose a reason for hiding this comment

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

I am very much for using the C11 atomics as well, We need to understand the effects of ++ operation.

last_id,
CExpr.uint64const(0)),
CExpr.call("__atomic_add_fetch", new List<>(last_id.adrOf(), CExpr.uint64const(1), new CIdent("__ATOMIC_SEQ_CST"))).ret());
last_id.incr().ret());
Copy link
Member

Choose a reason for hiding this comment

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

I think this might not be atomic, if there is no C function for this, you will need to do something like

  do
    {
      o = last_id;
    }
  while (compare_and_swap(&last_id, o, o+1) != o;

But maybe the C11 standard says something different.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe this to be atomic:

#include <stdio.h>
#include <stdint.h>
#include <stdlib.h>
#include <stdatomic.h>

int main()
{
    static atomic_uint_least64_t test = 0;
    ++test;
}
main:                                   # @main
        push    rbp
        mov     rbp, rsp
        lock            inc     qword ptr [rip + main.test]
        xor     eax, eax
        pop     rbp
        ret

https://godbolt.org/z/GbYj78Mvb

@fridis fridis merged commit 137bca1 into tokiwa-software:main Feb 22, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C backend related to the C backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants