-
Notifications
You must be signed in to change notification settings - Fork 177
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
Initial Atomic<T>
type implementation.
#5125
Conversation
docs/proposals/003-atomic-t.md
Outdated
enum MemoryOrder | ||
{ | ||
Relaxed = $(kIRMemoryOrder_Relaxed), | ||
SeqCst = $(kIRMemoryOrder_SeqCst), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW Vulkan doesn't actually support sequentially consistent atomics. We do support AcquireRelease which is close, but doesn't support some obscure use cases. I think it would be fine to only have Relaxed for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. SeqCst
was added because metal supports it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can have AcquireRelease here and map it to SeqCst on metal.
There was a problem hiding this 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.
__intrinsic_op($(kIROp_AtomicInc)) | ||
[__ref] T increment(MemoryOrder order = MemoryOrder.Relaxed); | ||
__intrinsic_op($(kIROp_AtomicDec)) | ||
[__ref] T decrement(MemoryOrder order = MemoryOrder.Relaxed); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would they return the original value too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, all atomic methods returns original values.
extension float : IArithmeticAtomicable {} | ||
extension half : IArithmeticAtomicable {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The interface definition is fine, because IArithmeticAtomicable is IAtomicable, and compareExchange is a IAtomicable method.
However this reminds me that the currently code emit logic for floating point atomics on HLSL is wrong. We should be using the InterlockedAddF32/F16 etc instead of InterlockedAdd.
I am going to merge this first, and then attempt to fix the floating point intrinsics on hlsl later. |
Closes #5084.
Todo: test new Atomic type for all existing targets as well as wgsl target.