Generate constructors for union and struct records

[serenity4]

Implements constructors for union structs, as discussed in #304, and also for struct records (in a very similar way).

I am unsure about struct records since fields may overlap, so any suggestions are welcome.

Because we use setproperty! on the pointer, which does most of the layout logic, I think this takes care of all possibilities including unions with offsets/different sizes, records with and without byte-aligned fields, records with unions, unions with records.

See below for a few examples of what the generated constructor looks like.

Union struct:

# definition
struct VkClearValue
    data::NTuple{16, UInt8}
end

# to see the layout
function Base.getproperty(x::Ptr{VkClearValue}, f::Symbol)
    f === :color && return Ptr{VkClearColorValue}(x + 0)
    f === :depthStencil && return Ptr{VkClearDepthStencilValue}(x + 0)
    return getfield(x, f)
end

# generated code from this PR

const __U_VkClearValue = Union{VkClearColorValue, VkClearDepthStencilValue}

function VkClearValue(val::__U_VkClearValue)
    ref = Ref{VkClearValue}()
    ptr = Base.unsafe_convert(Ptr{VkClearValue}, ref)
    if val isa VkClearColorValue
        ptr.color = val
    elseif val isa VkClearDepthStencilValue
        ptr.depthStencil = val
    end
    ref[]
end

Struct record:

# definition
struct VkAccelerationStructureInstanceKHR
    data::NTuple{64, UInt8}
end

# to see the layout
function Base.getproperty(x::Ptr{VkAccelerationStructureInstanceKHR}, f::Symbol)
    f === :transform && return Ptr{VkTransformMatrixKHR}(x + 0)
    f === :instanceCustomIndex && return Ptr{UInt32}(x + 48)
    f === :mask && return Ptr{UInt32}(x + 51)
    f === :instanceShaderBindingTableRecordOffset && return Ptr{UInt32}(x + 52)
    f === :flags && return Ptr{VkGeometryInstanceFlagsKHR}(x + 55)
    f === :accelerationStructureReference && return Ptr{UInt64}(x + 56)
    return getfield(x, f)
end

# generated code

function VkAccelerationStructureInstanceKHR(transform::VkTransformMatrixKHR, instanceCustomIndex::UInt32, mask::UInt32, instanceShaderBindingTableRecordOffset::UInt32, flags::VkGeometryInstanceFlagsKHR, accelerationStructureReference::UInt64)
    ref = Ref{VkAccelerationStructureInstanceKHR}()
    ptr = Base.unsafe_convert(Ptr{VkAccelerationStructureInstanceKHR}, ref)
    ptr.transform = transform
    ptr.instanceCustomIndex = instanceCustomIndex
    ptr.mask = mask
    ptr.instanceShaderBindingTableRecordOffset = instanceShaderBindingTableRecordOffset
    ptr.flags = flags
    ptr.accelerationStructureReference = accelerationStructureReference
    ref[]
end

[serenity4]

I think the idea is actually right for struct records, but the setproperty!/getproperty methods can be modified to not overflow into other fields for both getting and setting a field. The following works as intended:

struct VkAccelerationStructureInstanceKHR
    data::NTuple{64, UInt8}
end

# no changes
function Base.getproperty(x::Ptr{VkAccelerationStructureInstanceKHR}, f::Symbol)
    f === :transform && return Ptr{VkTransformMatrixKHR}(x + 0)
    f === :instanceCustomIndex && return Ptr{UInt32}(x + 48)
    f === :mask && return Ptr{UInt32}(x + 51)
    f === :instanceShaderBindingTableRecordOffset && return Ptr{UInt32}(x + 52)
    f === :flags && return Ptr{VkGeometryInstanceFlagsKHR}(x + 55)
    f === :accelerationStructureReference && return Ptr{UInt64}(x + 56)
    return getfield(x, f)
end

function unsafe_load_overlapping(ptr, nbytes)
    T = eltype(ptr)
    bytes = Base.unsafe_convert(Ptr{UInt8}, ptr)
    arr = zeros(UInt8, sizeof(T))
    arr[1:nbytes] .= unsafe_wrap(Array, bytes, nbytes)
    Base.reinterpret(T, arr)[]
end

function Base.getproperty(x::VkAccelerationStructureInstanceKHR, f::Symbol)
    r = Ref{VkAccelerationStructureInstanceKHR}(x)
    ptr = Base.unsafe_convert(Ptr{VkAccelerationStructureInstanceKHR}, r)
    fptr = getproperty(ptr, f)
    begin
        # change here; I didn't do the other branch but we could employ a similar logic
        if fptr isa Ptr
            GC.@preserve r begin
                # use unsafe_load_overlapping only for fields that overlap with others
                # (all the following fields are UInt32, so 4 bytes in size)
                if f === :instanceCustomIndex
                    unsafe_load_overlapping(fptr, 3)
                elseif f === :mask
                    unsafe_load_overlapping(fptr, 1)
                elseif f === :instanceShaderBindingTableRecordOffset
                    unsafe_load_overlapping(fptr, 3)
                elseif f === :flags
                    unsafe_load_overlapping(fptr, 1)
                else
                    unsafe_load(fptr)
                end
            end
        else
            (baseptr, offset, width) = fptr
            ty = eltype(baseptr)
            i8 = GC.@preserve(r, unsafe_load(baseptr))
            bitstr = bitstring(i8)
            sig = bitstr[(end - offset) - (width - 1):end - offset]
            zexted = lpad(sig, 8 * sizeof(ty), '0')
            return parse(ty, zexted; base = 2)
        end
    end
end

# this one was reworked a bit. Instead of using `unsafe_store!` (which stores all the bytes at once)
# we use `unsafe_copyto!` to store just the number of bytes we need (truncating in case of overflow)
# behavior is modified only for fields that overlap with others
function Base.setproperty!(x::Ptr{VkAccelerationStructureInstanceKHR}, f::Symbol, v)
    vref = Ref(v)
    GC.@preserve vref begin
        vptr = Base.unsafe_convert(Ptr{eltype(vref)}, vref)
        vbytes = Base.unsafe_convert(Ptr{UInt8}, vptr)
        xbytes = Base.unsafe_convert(Ptr{UInt8}, getproperty(x, f))
        if f === :transform
            unsafe_copyto!(xbytes, vbytes, 48)
        elseif f === :instanceCustomIndex
            unsafe_copyto!(xbytes, vbytes, 3)
        elseif f === :mask
            unsafe_copyto!(xbytes, vbytes, 1)
        elseif f === :instanceShaderBindingTableRecordOffset
            unsafe_copyto!(xbytes, vbytes, 3)
        elseif f === :flags
            unsafe_copyto!(xbytes, vbytes, 1)
        elseif f === :accelerationStructureReference
            unsafe_copyto!(xbytes, vbytes, 8)
        end
    end
end

# no changes from the above comment
function VkAccelerationStructureInstanceKHR(transform::VkTransformMatrixKHR, instanceCustomIndex::UInt32, mask::UInt32, instanceShaderBindingTableRecordOffset::UInt32, flags::VkGeometryInstanceFlagsKHR, accelerationStructureReference::UInt64)
    ref = Ref{VkAccelerationStructureInstanceKHR}()
    ptr = Base.unsafe_convert(Ptr{VkAccelerationStructureInstanceKHR}, ref)
    ptr.transform = transform
    ptr.instanceCustomIndex = instanceCustomIndex
    ptr.mask = mask
    ptr.instanceShaderBindingTableRecordOffset = instanceShaderBindingTableRecordOffset
    ptr.flags = flags
    ptr.accelerationStructureReference = accelerationStructureReference
    ref[]
end

[serenity4]

Maybe we should tackle those changes in getproperty/setproperty! in a separate issue/PR, since it may be a bit involved implementation-wise. For this PR you can skip the previous comment, since I haven't implemented the changes.

[Gnimuc]

const __U_VkClearValue = Union{VkClearColorValue, VkClearDepthStencilValue}
function VkClearValue(val::__U_VkClearValue)
    ref = Ref{VkClearValue}()
    ptr = Base.unsafe_convert(Ptr{VkClearValue}, ref)
    if val isa VkClearColorValue
        ptr.color = val
    elseif val isa VkClearDepthStencilValue
        ptr.depthStencil = val
    end
    ref[]
end

For unions, I'm considering generating something like:

function VkClearValue(val::VkClearColorValue)
    ref = Ref{VkClearValue}()
    ptr = Base.unsafe_convert(Ptr{VkClearValue}, ref)
    ptr.color = val
    ref[]
end

function VkClearValue(val::VkClearDepthStencilValue)
    ref = Ref{VkClearValue}()
    ptr = Base.unsafe_convert(Ptr{VkClearValue}, ref)
    ptr.depthStencil = val
    ref[]
end

This version uses multiple-dispatch at the call site, but the generated codes are a little bit lengthy. Anyway, it's just a matter of code style, and the low-level IR codes are the same.

[Gnimuc]

ref = Ref{VkClearValue}() implicitly assumes that VkClearValue is an immutable isbits type. The TweakMutability pass is the only pass that can tweak the mutability, and it doesn't work for any memory-pool-like structures, so this assumption is always valid.

However, depending on the API of the C library, it may be unnecessary to generate constructors for all of these memory-pool-like structures, so I believe we'd better make this feature optional by adding an option like:

Clang.jl/gen/generator.toml

Lines 135 to 136 in d691ad7

	# generate getproperty/setproperty! methods for the types in the following list
	field_access_method_list = []

[serenity4]

This version uses multiple-dispatch at the call site, but the generated codes are a little bit lengthy. Anyway, it's just a matter of code style, and the low-level IR codes are the same.

I didn't use multiple dispatch because the differences are only in a small portion of the code (the if branch) and thought it would add redundancy. However I recognize that it would be more straightforward (at least from a readability perspective) to use multiple dispatch, so that the branching part gets removed. I don't have a preference, so I can go with the multiple dispatch style if you prefer.

However, depending on the API of the C library, it may be unnecessary to generate constructors for all of these memory-pool-like structures, so I believe we'd better make this feature optional

Good point. We could make the option true by default (when not specified), and potentially have it specified as a list of excluded types. It can be the opposite if you think that this feature will be disabled more often than it will be enabled. We could call it something like blacklist_record_layout_constructors.

[Gnimuc]

However I recognize that it would be more straightforward (at least from a readability perspective) to use multiple dispatch, so that the branching part gets removed. I don't have a preference, so I can go with the multiple dispatch style if you prefer.

If you don't mind, we can support both of these two styles by adding another option. 😜

We could make the option true by default (when not specified), and potentially have it specified as a list of excluded types. It can be the opposite if you think that this feature will be disabled more often than it will be enabled.

I'd say we keep it secret in this PR just like what we did in #303. I also think we should make this feature enabled by default in the final 0.14 release, but let's make the decision later, in another issue, and hear the voice from other users.

We could call it something like blacklist_record_layout_constructors.

Better not use the word "blacklist", see #302.

[serenity4]

I ended up making an option add_record_constructors that can be set to true, false (default) or a list with specific symbols.

I also made an option for whether or not to generate a single constructor for unions (surprisingly named union_single_constructor) that is turned off by default. The generated code is identical as before if it is on, and if it is off we have multiple constructors defined as follows:

const __U_VkClearValue = Union{VkClearColorValue, VkClearDepthStencilValue}

function VkClearValue(color::VkClearColorValue)
    ref = Ref{VkClearValue}()
    ptr = Base.unsafe_convert(Ptr{VkClearValue}, ref)
    ptr.color = color
    ref[]
end

function VkClearValue(depthStencil::VkClearDepthStencilValue)
    ref = Ref{VkClearValue}()
    ptr = Base.unsafe_convert(Ptr{VkClearValue}, ref)
    ptr.depthStencil = depthStencil
    ref[]
end

Note that the constant is not used, but it may be useful for uses that want to dispatch on it, and I think it's better to have the same symbols regardless of the options we chose.

[Gnimuc]

Thanks for the PR!