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

Atomic operations. #523

Merged

Conversation

Apprentice-Alchemist
Copy link
Contributor

The JIT implementation is based on what GCC emits for it's intrinsics.
The C implementation is copy pasted from what I did for hxcpp.

See HaxeFoundation/haxe#10610

@ncannasse
Copy link
Member

I don't think this requires some specific opcodes, especially since the C version - which is supposed to be faster - is based on library function calls.

@Apprentice-Alchemist
Copy link
Contributor Author

Good point about library calls on MSVC, I should replace those with the intrinsics.

But for GCC/Clang it already uses instrinsics, which do not call library functions: Godbolt

@Simn
Copy link
Member

Simn commented Aug 2, 2022

What's the status here? I'm willing to merge the hxcpp PR, but I'm unsure where we're at for HL.

@ncannasse
Copy link
Member

That's a lot of code. I'm curious what's the rationale for supporting all 8/16/32/64 bits int values ? I think 32 + ptr only would be good enough.
Also, if the hl_atomic inlines are only meant to support HL/C, maybe they should be in hlc.h and not hl.h ?

@Apprentice-Alchemist
Copy link
Contributor Author

That's a lot of code. I'm curious what's the rationale for supporting all 8/16/32/64 bits int values ? I think 32 + ptr only would be good enough.

Hashlink has 8/16/32/64 bit int types, so I thought it made sense to already implement atomic operations for all of them even though there are no AtomicI8/16/64 types (yet).

Also, if the hl_atomic inlines are only meant to support HL/C, maybe they should be in hlc.h and not hl.h ?

Having them in hl.h could be useful if someone needs atomic operations when, for example, writing a hdll.

@ncannasse
Copy link
Member

I think we should keep things more simple with only int32 atomics and using primitives instead of opcodes.

@Apprentice-Alchemist
Copy link
Contributor Author

I've removed the 8/16/64 bit atomics, and switched everything to primitives.
I have kept the pointer atomics, it'd be a shame if AtomicObject were only implemented for Java.

@Simn
Copy link
Member

Simn commented Nov 7, 2022

@ncannasse Should this be merged? I'm happy to handle the hxcpp and Haxe side of this, but we need a decision for HL first.

@ncannasse
Copy link
Member

I could merge the PR but it still contains some JIT support leftovers.

@Apprentice-Alchemist
Copy link
Contributor Author

The leftovers from JIT support have been removed.

@ncannasse
Copy link
Member

Last change before merge : I think we should directly add atomics to threads.c ;)

@ncannasse ncannasse merged commit 67a6a9c into HaxeFoundation:master Nov 20, 2022
@ncannasse
Copy link
Member

Thanks !

crazyjul pushed a commit to playdigious/hashlink that referenced this pull request Jan 18, 2024
@Apprentice-Alchemist Apprentice-Alchemist deleted the feature/atomics branch June 4, 2024 12:01
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.

3 participants