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
2.01k stars 556 forks source link

MSL: Unsized array of images is not supported in MSL #2112

Open rajivmg opened 1 year ago

rajivmg commented 1 year ago

We are converting HLSL to SPIR-V using DXC and then SPIR-V to MSL with SPIRV-Cross. We are getting an error msg Unsized array of images is not supported in MSL. Though it should be supported if SPIRV-Cross has support for Metal Argument Buffers and Bindless.

For debugging, I converted the SPIR-V to VK glsl. And it seems this line is causing the problem layout(set = 30, binding = 0) uniform texture2D CommonBindlessResources_textures[];

The original HLSL line was Texture2D<float4> CommonBindlessResources_textures[] : register(t0 , space30 );

As far as I understand the MSL conversion should be something like:

struct CommonBindlessResources_texturesStruct
{
    texture2d<float> textures;
};

pixelShaderEntryPoint(CommonBindlessResources_texturesStruct* CommonBindlessResources_textures [[buffer(0)]])
{

}

We are compiling spirv to msl with following options:

    spirv_cross::CompilerMSL::Options options;
    options.platform = spirv_cross::CompilerMSL::Options::Platform::iOS;
    options.msl_version = spirv_cross::CompilerMSL::Options::make_msl_version(2, 3);
    options.ios_use_simdgroup_functions = true;
    options.ios_support_base_vertex_instance = true;
    options.enable_decoration_binding = true;
    options.texture_buffer_native = true;
    options.argument_buffers = true;

Any ideas what's wrong? Will I have to modify spirv-cross and add support for this conversion?

HansKristian-Work commented 1 year ago

SPIR-V allows descriptor sets to have some bindings that are not unsized array and then followed by one unsized array, e.g.

layout(set = 0, binding = 0) uniform texture2D Tex0;
layout(set = 0, binding = 1) uniform UBO { };
layout(set = 0, binding = 2) uniform texture2D Bindless[];

Plain array in MSL would not work. I think the trick to emit array size of [1] would work? Set = 30 is also questionable. The limit in MSL is 8 iirc.

rajivmg commented 1 year ago

Plain array in MSL would not work. I think the trick to emit array size of [1] would work? Set = 30 is also questionable. The limit in MSL is 8 iirc.

Could you please elaborate a little more on the trick to emit array size of [1]? I don't understand what you meant by plain array.

HansKristian-Work commented 1 year ago

CommonBindlessResources_texturesStruct* CommonBindlessResources_textures [[buffer(0)]]

Would only work if the descriptor set contains a single unsized descriptor array, since arrays can decay to pointers.

The descriptor buffer might have to look something like this in the more general case however:

struct DescBuffer
{
   texture2d<float> Tex0 [[id(0)]];
   constant UBO *ubo [[id(1)]];
   texture2d<float> Bindless[1 /* Actually unsized C99 style, but MSL does not support that */] [[id(2)]];
};

We have to do similar things for SSBOs currently.

rajivmg commented 1 year ago

CommonBindlessResources_texturesStruct* CommonBindlessResources_textures [[buffer(0)]]

Would only work if the descriptor set contains a single unsized descriptor array, since arrays can decay to pointers.

I understand. But how do I make SPIRV-Cross emit correct MSL code? Because right now we are getting Unsized array of images is not supported in MSL. error msg.

HansKristian-Work commented 1 year ago

Try replacing the unsized array with some large constant value (e.g. 500k). If that code ends up doing what you expect, we can probably rewrite unsized descriptor arrays the same way in SPIRV-Cross.

rajivmg commented 1 year ago

Try replacing the unsized array with some large constant value (e.g. 500k). If that code ends up doing what you expect, we can probably rewrite unsized descriptor arrays the same way in SPIRV-Cross.

Thanks, I will check tomorrow. But I don't think this solution will work for us. In the meanwhile, would there be any downsides of using array of textures instead of the actual bindless resources feature that uses the pointer syntax as described on this page https://developer.apple.com/documentation/metal/buffers/about_argument_buffers?

rajivmg commented 1 year ago

I tried using large array of textures, but it didn't work.

This was the source HLSL.

struct VSOut
{
    float4 pos : SV_POSITION;
    float2 texCoord0 : TEXCOORD0;
};

SamplerState smp : register(s0, space0);
Texture2D<float> simpleBindlessTextures[10000] : register(t0, space1);

float4 fragmentMain(in VSOut input) : SV_Target
{
    float4 tex = simpleBindlessTextures[3].Sample(smp, input.texCoord0, 0);
    return float4(tex.rgb, 1.0);
}

Converted that shader to SPIRV using DXC command dxc "F:\SPIRV-Cross_Build\simple_bindless.hlsl" -spirv -T ps_6_1 -E fragmentMain -Fo "F:\SPIRV-Cross_Build\simple_bindless.spirv"

Then using SPIRV-Cross converted that to MSL using command spirv-cross.exe F:\SPIRV-Cross_Build\simple_bindless.spirv --msl --msl-ios --msl-version 20100 --msl-argument-buffers --msl-decoration-binding > simple_bindless.metal`

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

using namespace metal;

struct spvDescriptorSetBuffer0
{
    sampler smp [[id(0)]];
};

struct spvDescriptorSetBuffer1
{
    array<texture2d<float>, 10000> simpleBindlessTextures [[id(0)]];
};

struct fragmentMain_out
{
    float4 out_var_SV_Target [[color(0)]];
};

struct fragmentMain_in
{
    float2 in_var_TEXCOORD0 [[user(locn0)]];
};

fragment fragmentMain_out fragmentMain(fragmentMain_in in [[stage_in]], constant spvDescriptorSetBuffer0& spvDescriptorSet0 [[buffer(0)]], constant spvDescriptorSetBuffer1& spvDescriptorSet1 [[buffer(1)]])
{
    fragmentMain_out out = {};
    out.out_var_SV_Target = float4(spvDescriptorSet1.simpleBindlessTextures[3].sample(spvDescriptorSet0.smp, in.in_var_TEXCOORD0, int2(0)).xxx, 1.0);
    return out;
}

Now when I compile the shader with Metal using command metal F:\SPIRV-Cross_Build\Debug\simple_bindless.metal. I'm getting error.

F:\Metal Developer Tools\ios\bin>metal F:\SPIRV-Cross_Build\Debug\simple_bindless.metal
F:\SPIRV-Cross_Build\Debug\simple_bindless.metal:26:140: error: size of constant buffer cannot exceed 65536
fragment fragmentMain_out fragmentMain(fragmentMain_in in [[stage_in]], constant spvDescriptorSetBuffer0& spvDescriptorSet0 [[buffer(0)]], constant spvDescriptorSetBuffer1& spvDescriptorSet1 [[buffer(1)]])
                                                                                                                                           ^
1 error generated.

Reducing the size of array to 7000, get rid of the error on this particular sample shader. But on production level complex shaders, I get errors of this type

F:\SPIRV-Cross_Build\Debug\simple_bindless.metal:16:103: error: cannot reserve 'texture' resource locations at index 0
fragment fragmentMain_out fragmentMain(fragmentMain_in in [[stage_in]], array<texture2d<float>, 7000> simpleBindlessTextures [[texture(0)]], sampler smp [[sampler(0)]])
                                                                                                      ^
1 error generated.

So I guess the other solution left is to implement proper unbound descriptor arrays using the pointer syntax.

HansKristian-Work commented 1 year ago

F:\Metal Developer Tools\ios\bin>metal F:\SPIRV-Cross_Build\Debug\simple_bindless.metal F:\SPIRV-Cross_Build\Debug\simple_bindless.metal:26:140: error: size of constant buffer cannot exceed 65536 fragment fragmentMain_out fragmentMain(fragmentMain_in in [[stage_in]], constant spvDescriptorSetBuffer0& spvDescriptorSet0 [[buffer(0)]], constant spvDescriptorSetBuffer1& spvDescriptorSet1 [[buffer(1)]]) ^ 1 error generated.

You need device address space for large descriptor buffers. constant only works with 64k. See CompilerMSL::set_argument_buffer_device_address_space().

HansKristian-Work commented 1 year ago

F:\SPIRV-Cross_Build\Debug\simple_bindless.metal:16:103: error: cannot reserve 'texture' resource locations at index 0 fragment fragmentMain_out fragmentMain(fragmentMain_in in [[stage_in]], array<texture2d, 7000> simpleBindlessTextures [[texture(0)]], sampler smp [[sampler(0)]]) ^ 1 error generated.

Is plain-old descriptors, and large arrays will never work there.

HansKristian-Work commented 1 year ago

So I guess the other solution left is to implement proper unbound descriptor arrays using the pointer syntax.

That's a last resort hack, because it cannot work in the general case unfortunately.

Try commented 1 year ago

So I guess the other solution left is to implement proper unbound descriptor arrays using the pointer syntax.

+1 vote for this approach. This map clan to DX12 and VK_DESCRIPTOR_BINDING_VARIABLE_DESCRIPTOR_COUNT_BIT unsized array C99 style, is UB when it comes to argument buffers and better to avoid.

HansKristian-Work commented 1 year ago

+1 vote for this approach. This map clan to DX12 and VK_DESCRIPTOR_BINDING_VARIABLE_DESCRIPTOR_COUNT_BIT unsized array C99 style, is UB when it comes to argument buffers and better to avoid.

Can you elaborate how this should work when a descriptor set consists of normal descriptors + variable descriptor count bit? The only way I can make pointer syntax work is if you opt-in to saying that the descriptor set only contains VARIABLE_DESCRIPTOR_COUNT. Then it can collapse to a pointer.

Try commented 1 year ago

Can you elaborate how this should work when a descriptor set consists of normal descriptors + variable descriptor count bit?

a. In case, if options.argument_buffers==false 'Just works', because every resource will have automatic binding.

b. In case, if options.argument_buffers==true Won't work as-is, yet this is consistent with HLSL backend, so probably also acceptable. Alternatively, last OpRuntimeArray in set, when there are other resources, can be decoupled into it's own auto-binding.

HansKristian-Work commented 1 year ago

Just to be clear, is the problem that the array size is 1 specifically? In Vulkan, you have to specify the maximum binding count for VARIABLE_DESCRIPTOR_COUNT, so it would be feasible to add an option to specify how large that array size should be. Normally, it could maybe just default to something large-ish, like 500k or 1M. Since raw pointers are allowed in descriptor buffers, it should be okay to only bind enough memory that is actually used.

can be decoupled into it's own auto-binding

Not really practical. Update-after-bind means all resources have to reside in an argument buffer.

Try commented 1 year ago

In Vulkan, you have to specify the maximum binding count for VARIABLE_DESCRIPTOR_COUNT, so it would be feasible to add an option to specify how large that array size should be.

Didn't know that, been expecting behavior similar to DX12, where application can specify D3D12_DESCRIPTOR_RANGE_TYPE::NumDescriptors = -1. Hm, this is very bad news for any kind of bindless usage.

Not really practical. Update-after-bind means all resources have to reside in an argument buffer.

I'm not following this. The is no interaction with update-after-bind, right?