Refactor Atomic#swap for reference-like types
#14402
Conversation
| address = atomicrmw(:xchg, pointerof(@value).as(LibC::SizeT*), LibC::SizeT.new(value.as(Void*).address), ordering) | ||
| Pointer(T).new(address).as(T) | ||
| address = atomicrmw(:xchg, pointerof(@value).as(typeof(@value.object_id)*), value.object_id, ordering) | ||
| Pointer(Void).new(address).as(T) |
There was a problem hiding this comment.
Unless I'm mistaken #object_id is always casted to 64-bit, whatever of the architecture (32 or 64-bit) which would lead to an expensive DWCAS on i686, and is likely disabled by LLVM by default anyway. I'm not sure about ARM.
I'd keep the cast as SizeT.
There was a problem hiding this comment.
Hm, good point. Maybe we would want #object_id to actually be of platform-specific width?
Crystal itself just doesn't have a SizeT type yet, so it's just been the default UInt64 for now.
However, thinking a bit more about this #object_id could be overriden by any type (for whatever reason). So taking the address directly is actually a bit more robust.
There was a problem hiding this comment.
Ideally, yes, #object_id should be arch-specific. Casting to Void* and using the address seems more robust to me.
But now I'm not sure we don't cast Pointer#address as UInt64 too...
edit: hence the explicit cast to SizeT.
There was a problem hiding this comment.
Yeah, the return type of Pointer#address is documented as UInt64. But I believe the underlying LLVMBuildPtrToInt actually casts to an integer of the same width as a pointer.
There was a problem hiding this comment.
LLVMBuildPtrToInt takes a target type, which is currently hardcoded to i64:
crystal/src/compiler/crystal/codegen/primitives.cr
Lines 796 to 798 in 723f829
There is no need to use
LibC::SizeThere since we could obtain the actual type with atypeof.