-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
WIP: make reinterpret
work on structs
#32660
Conversation
if isprimitivetype(Out) && isprimitivetype(In) | ||
return bitcast(Out, x) | ||
elseif struct_subpadding(Out, In) | ||
in = Ref{In}(x) |
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.
Couldn't we simply
GC.@preserve in begin return unsafe_load(unsafe_convert(Ptr{Out}, in)) end
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, that’s probably better
ptr_in = unsafe_convert(Ptr{In}, in) | ||
out = Ref{Out}() | ||
ptr_out = unsafe_convert(Ptr{Out}, out) | ||
GC.@preserve in out begin |
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.
Shouldn't in
and out
be preserved before getting the pointer?
Since struct layout is not generally defined (other than for C-compatible isbits types), we've previously felt that |
In that case, is an |
I guess you'd need to check that both types have the same padding? Though I see you have a note about not being able to check the input's padding, can you say more about that? |
Sorry, yes I remember the issue now. We need to check that any input padding bits are also output padding bits. Maybe even vice-versa, just in case there is some UB there. I’m not sure if there’s any function lying around that can do that or to help see the padding bits? |
I think amending
The only consumer of |
I think this works?
There'd still be the Bool trap representation issue from #43035 . I don't think This PR actually handles what I was trying to do in #43035 but better. You can even do:
and just get the Int64 directly. |
I suppose one issue might be that
Changing the It could be that reinterpreting to something with less padding should be always allowed (as the current PR mentions in a comment), but it would still be useful to have a potential round-trip available. Perhaps it could check that the bytes that would go into the padding are all zeroed? |
I believe we need an unsafe (narrow contract API), lower-level, and less magic version of For such use case, we need a very predictable behavior and need to avoid the "magic behavior" of Here's a sketch of the API
Implementation is trivial since I'm not suggesting to detect padding inconsistency: function unsafe_bitcast(::Type{T}, x::S) where {T,S}
isconcretetype(T) || throw(ArgumentError("output type $T is not concrete"))
datatype_pointerfree(T) ||
throw(ArgumentError("output type $T may contain a boxed object"))
datatype_pointerfree(S) ||
throw(ArgumentError("input type $S may contain a boxed object"))
sizeof(T) == sizeof(S) || throw(ArgumentError("different input and output sizes"))
output = Ref{T}()
input = Ref{S}(x)
GC.@preserve output input begin
po = Ptr{UInt8}(pointer_from_objref(output))
pi = Ptr{UInt8}(pointer_from_objref(input))
_memcpy!(po, pi, sizeof(S))
end
return output[]
end |
Could you expand on that a little? I'm not familiar with GPU programming and I don't immediately see why |
If Should |
The emphasis is on "supporting rich pointer-free immutable types" not on GPU. What I wanted to say was that, if I need to use objects containing
Julia's
Do you mean to pack bytes when the in-memory representation contain pads? I can't think of the usage of this for me personally. But, if it has some use somewhere, I can imagine having it in Base make sense since it's not clear how to access all these data layout information without touching the internals. |
Yeah. There were a couple people who raised a desire to convert tuples of various isbitstypes to tuples of other isbitstypes, which is how I got interested in this PR. |
OK, since it sounds like the use case of |
I believe I have a solution that implements my more general proposal. Is it possible to do a pull request to a pull request? https://github.com/BioTurboNick/julia/tree/reinterpret-all What I've added: Simple reinterpretations are still ~1 ns; packed-to-padded and vice versa ~14+ ns; mismatched padded-to-padded is 40+ ns, all without allocations. A remaining issue is Bool trap representations; however, the current The rationale I'm working with is that the high-level representation of data should be agnostic to the implementation details of the type. So there shouldn't be any logical barrier to converting a bitstype tuple or struct with fields that add up to 64 bits to any other tuple or struct with fields that add up to 64 bits, for example. |
Since "trap" values are not exclusive to booleans (as the proposed docstring by @tkf mentions, no constructor guarantees are observed), booleans could serve as a good example in the documentation to illustrate why people still have to check whether their desired bitcast is valid for their usecase or not. I still don't believe it should check anything/error here, since we have |
Gotcha. Looks like the bool issue is best left for #34909 |
Not sure if this is still useful, but #43035 was asking for invalid floats from reinterpret. Here are some examples: julia> typemax(Int64)
9223372036854775807
julia> reinterpret(Float64, typemax(Int64))
NaN
julia> reinterpret(Int64, NaN64)
9221120237041090560
julia> reinterpret(Float64, typemax(Int64)-1)
NaN
julia> reinterpret(Float64, typemax(Int64)-100)
NaN |
Thanks, but by invalid, we mean something that would have undefined behavior because the bit representation was not considered valid. NaNs are all valid bit patterns for Float64 and are processed as such. |
That's in contrast with Bools, which are only considered valid (in Julia/LLVM) if just the first bit is set or all bits are zero. Any other combination of values can lead to unexpected behavior because the compiler assumes the other bits in a Bool are all 0s. |
Looks like that bool issue is about to be fixed by #45689 ? |
It's only booleans though that are affected by this - other kinds of constructor constraints still don't get checked. In effect, the fix just looses us a convenient example with a builtin datatype 🤷 |
I think it's clear that the compiler considers the high 7 bits to be padding and is free to assume they're 0s, so arguably it's more in line with padding concerns than invalid bit patterns within valid bits. This creates undefined behavior that works sometimes and not in others. That is:
This struct considers any other values to be invalid Can we construct an example where behavior would be undefined, and not just wrong? |
Currently,
reinterpret
works on primitive types through thebitcast
intrinsic and as a (inner!) constructor forReinterpretArray
. In both older array element type casting and the newerReinterpretArray
implementation, it's possible to reinterpret arrays withisbits
struct fields as otherisbits
struct fields, making it more generic than the non-array version.This PR is my attempt to copy the way Keno implemented
getindex
onReinterpretArray
to makereinterpret
also work on structs. Semantically, we copy the value to aRef
and copy the bits to aRef
of a different type and load that. Fortnately there is enough codegen optimization such that, like theReinterpretArray
case, the overhead is (mostly) removed in the resultant LLVM/native code.This is work in progress - but I would appreciate early feedback from knowing people on the approach taken in the new
reinterpret
method.reinterpret
method.