KhronosGroup / SPIRV-Cross

SPIRV-Cross is a practical tool and library for performing reflection on SPIR-V and disassembling SPIR-V back to high level languages.
Apache License 2.0
1.96k stars 549 forks source link

MSL: Add support for overlapping bindings. #2292

Closed js6i closed 2 months ago

js6i commented 3 months ago

This adds support for bindings which share the same DescriptorSet/Binding pair.

The motivating example is vkd3d, which uses overlapping arrays of resources to emulate D3D12 descriptor tables. The generated MSL argument buffer only includes the first resource (in this example 't0'):

struct spvDescriptorSetBuffer2
{
    array<texture2d<float>, 499968> t0 [[id(0)]];
    // Overlapping binding: array<texture3d<float>, 499968> t2 [[id(0)]];
};

When t2 is referenced, we cast the instantiated member:

float4 r1 = spvDescriptorSet2.t0[_79].sample(...);
float4 r2 = (*(constant array<texture3d<float>, 499968>*)&spvDescriptorSet2.t0)[_97].sample(...);
cdavis5e commented 3 months ago

Honestly, I'm just amazed the compiler will let you do this, and yet won't let you use unions in an argument buffer struct.

js6i commented 3 months ago

Yeah.. I don't understand what the CI compiler is complaining about. It builds cleanly here with arm64-apple-darwin23.3.0 and gcc 13.2.0-4ubuntu3.

HansKristian-Work commented 3 months ago

The idea seems fine, but this also seems kinda incomplete. To have good coverage for typical EXT_mutable_descriptor_type scenarios, I would expect to see tests for:

The latter 4 here might already be covered by existing work I did, but needs to be sanity checked.

js6i commented 3 months ago

Arrays of SSBO's look broken:

layout(set = 0, binding = 0) buffer B0 { float v; } b0[8]; // I guess, I haven't seen this in the wild

Translates to this:

struct spvDescriptorSetBuffer0
{
    device B0* b0 [[id(0)]][8];
};

Unsized array hack has a similar problem:

    spvDescriptor<texture2d<float>> t0 [[id(0)]][1] /* unsized array hack */;

A case that looks fine:

#version 450
#extension GL_EXT_samplerless_texture_functions : require
layout(local_size_x = 1, local_size_y = 1, local_size_z = 1) in;

layout(set = 0, binding = 0) uniform samplerShadow s0[8];
layout(set = 0, binding = 0) uniform texture2D t0[8];
layout(set = 0, binding = 0) uniform utexture2D t1[8];
layout(set = 0, binding = 0) uniform itexture2D t2[8];

vec4 r;

void main()
{
    r = textureLod(sampler2D(t0[0u], s0[3u]), vec4(0.0).xy, 0.0);
    r.x = uintBitsToFloat(texelFetch(t1[1u], ivec2(0), 0).x);
    r.y =  intBitsToFloat(texelFetch(t2[2u], ivec2(0), 0).x);
}

Translates to:

#include <metal_stdlib>
#include <simd/simd.h>

using namespace metal;

struct spvDescriptorSetBuffer0
{
    array<texture2d<float>, 8> t0 [[id(0)]];
    // Overlapping binding: array<texture2d<uint>, 8> t1 [[id(0)]];
    // Overlapping binding: array<texture2d<int>, 8> t2 [[id(0)]];
    // Overlapping binding: array<sampler, 8> s0 [[id(0)]];
};

kernel void main0(constant spvDescriptorSetBuffer0& spvDescriptorSet0 [[buffer(0)]])
{
    float4 r = spvDescriptorSet0.t0[0].sample((*(constant array<sampler, 8>*)&spvDescriptorSet0.t0)[3], float2(0.0), level(0.0));
    r.x = as_type<float>((*(constant array<texture2d<uint>, 8>*)&spvDescriptorSet0.t0)[1].read(uint2(int2(0)), 0).x);
    r.y = as_type<float>((*(constant array<texture2d<int>, 8>*)&spvDescriptorSet0.t0)[2].read(uint2(int2(0)), 0).x);
}

Are those the kind of tests you want to see?

The functionality is useful for vkd3d even without EXT_mutable_descriptor_type and the above bugs look not really related to what's currently in this PR. I can look into fixing these though.

One thing that is related is the address space. What would be the right way to make it emit the correct one? Somehow change type_to_glsl to take argument_buffer_device_storage_mask into account, or use something else..?

HansKristian-Work commented 3 months ago

argument_buffer_device_storage_mask

That one should be handled explicitly on the outside I think, since it's not part of type system at all. When you emit the alias you look up descriptor set and check which address space it has.

Are those the kind of tests you want to see?

Yes. I'll probably have to do some work myself to flush out more details with descriptor aliasing, but there should at least be healthy test coverage by this PR to make sure it's a reasonable direction.

Translates to this:

Looks fine to me?

js6i commented 3 months ago

Looks fine to me?

You're right, for some reason I assumed the [[]] annotation always goes at the end..

js6i commented 3 months ago

I added a couple hacks that probably aren't great.

HansKristian-Work commented 2 months ago

Merged locally and improving things before merging upstream. Think I figured out how to make this neater.