MoltenVK 1.2.11 breaks vkdoom

DUOLabs333 commented 2 weeks ago

This used to be a comment under #2146, but I thought it would be better as its own issue:

1.2.11 (up to 78396b7) breaks vkdoom, regardless of whether Metal argument buffers are used or not --- the error messages are different though (vkdoom worked on 1.2.9 with MVK_CONFIG_USE_METAL_ARGUMENT_BUFFERS=1). From using Actions artifacts, I have determined that the problem existed since at least 96f9d89: MVK_CONFIG_USE_METAL_ARGUMENT_BUFFERS=0:

[mvk-error] VK_ERROR_INITIALIZATION_FAILED: Shader library compile failed (Error code 3):
Compilation failed: 

program_source:429:12: warning: unused variable 'ray_end'
    float3 ray_end = origin + (dir * tmax);
           ^
program_source:587:464: error: cannot reserve 'texture' resource locations at index 0
fragment main0_out main0(main0_in in [[stage_in]], device SurfaceIndexBuffer& _608 [[buffer(1)]], device SurfaceBuffer& _604 [[buffer(2)]], device LightBuffer& _941 [[buffer(3)]], device LightIndexBuffer& _945 [[buffer(4)]], device PortalBuffer& _696 [[buffer(5)]], device ConstantsBuffer& _904 [[buffer(6)]], device NodeBuffer& _131 [[buffer(7)]], device VertexBuffer& _285 [[buffer(8)]], device ElementBuffer& _289 [[buffer(9)]], array<texture2d<float>, 16536> textures [[texture(0)]], array<sampler, 16536> texturesSmplr [[sampler(0)]])
                                                                                                                                                                                                                                                                                                                                                                                                                                                                               ^
program_source:587:511: error: cannot reserve 'sampler' resource locations at index 0
fragment main0_out main0(main0_in in [[stage_in]], device SurfaceIndexBuffer& _608 [[buffer(1)]], device SurfaceBuffer& _604 [[buffer(2)]], device LightBuffer& _941 [[buffer(3)]], device LightIndexBuffer& _945 [[buffer(4)]], device PortalBuffer& _696 [[buffer(5)]], device ConstantsBuffer& _904 [[buffer(6)]], device NodeBuffer& _131 [[buffer(7)]], device VertexBuffer& _285 [[buffer(8)]], device ElementBuffer& _289 [[buffer(9)]], array<texture2d<float>, 16536> textures [[texture(0)]], array<sampler, 16536> texturesSmplr [[sampler(0)]])
                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                              ^
.
[mvk-error] VK_ERROR_INVALID_SHADER_NV: Fragment shader function could not be compiled into pipeline. See previous logged error.

MVK_CONFIG_USE_METAL_ARGUMENT_BUFFERS=1:

[mvk-error] SPIR-V to MSL conversion error: Argument buffer resource base type could not be determined. When padding argument buffer elements, all descriptor set resources must be supplied with a base type by the app.
[mvk-error] VK_ERROR_INVALID_SHADER_NV: Fragment shader function could not be compiled into pipeline. See previous logged error.

DUOLabs333 commented 2 weeks ago

However, to be fair, vkdoom at no point passes validation, so the fact it worked before was probably a lucky accident.

The validation errors:

VUID-VkPipelineLayoutCreateInfo-descriptorType-03022(ERROR / SPEC): msgNum: -658548338 - Validation Error: [ VUID-VkPipelineLayoutCreateInfo-descriptorType-03022 ] | MessageID = 0xd8bf598e | vkCreatePipelineLayout():  max per-stage sampler bindings count (16536) exceeds device maxPerStageDescriptorUpdateAfterBindSamplers limit (1024). The Vulkan spec states: The total number of descriptors with a descriptorType of VK_DESCRIPTOR_TYPE_SAMPLER and VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER accessible to any given shader stage across all elements of pSetLayouts must be less than or equal to VkPhysicalDeviceDescriptorIndexingProperties::maxPerStageDescriptorUpdateAfterBindSamplers (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-VkPipelineLayoutCreateInfo-descriptorType-03022)
    Objects: 0
VUID-VkPipelineLayoutCreateInfo-pSetLayouts-03036(ERROR / SPEC): msgNum: -662943472 - Validation Error: [ VUID-VkPipelineLayoutCreateInfo-pSetLayouts-03036 ] | MessageID = 0xd87c4910 | vkCreatePipelineLayout():  sum of sampler bindings among all stages (16536) exceeds device maxDescriptorSetUpdateAfterBindSamplers limit (1024). The Vulkan spec states: The total number of descriptors of the type VK_DESCRIPTOR_TYPE_SAMPLER and VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER accessible across all shader stages and across all elements of pSetLayouts must be less than or equal to VkPhysicalDeviceDescriptorIndexingProperties::maxDescriptorSetUpdateAfterBindSamplers (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-VkPipelineLayoutCreateInfo-pSetLayouts-03036)
    Objects: 0
VUID-VkPipelineLayoutCreateInfo-descriptorType-03022(ERROR / SPEC): msgNum: -658548338 - Validation Error: [ VUID-VkPipelineLayoutCreateInfo-descriptorType-03022 ] | MessageID = 0xd8bf598e | vkCreatePipelineLayout():  max per-stage sampler bindings count (16536) exceeds device maxPerStageDescriptorUpdateAfterBindSamplers limit (1024). The Vulkan spec states: The total number of descriptors with a descriptorType of VK_DESCRIPTOR_TYPE_SAMPLER and VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER accessible to any given shader stage across all elements of pSetLayouts must be less than or equal to VkPhysicalDeviceDescriptorIndexingProperties::maxPerStageDescriptorUpdateAfterBindSamplers (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-VkPipelineLayoutCreateInfo-descriptorType-03022)
    Objects: 0
VUID-VkPipelineLayoutCreateInfo-pSetLayouts-03036(ERROR / SPEC): msgNum: -662943472 - Validation Error: [ VUID-VkPipelineLayoutCreateInfo-pSetLayouts-03036 ] | MessageID = 0xd87c4910 | vkCreatePipelineLayout():  sum of sampler bindings among all stages (16536) exceeds device maxDescriptorSetUpdateAfterBindSamplers limit (1024). The Vulkan spec states: The total number of descriptors of the type VK_DESCRIPTOR_TYPE_SAMPLER and VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER accessible across all shader stages and across all elements of pSetLayouts must be less than or equal to VkPhysicalDeviceDescriptorIndexingProperties::maxDescriptorSetUpdateAfterBindSamplers (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-VkPipelineLayoutCreateInfo-pSetLayouts-03036)
    Objects: 0
VUID-VkPipelineLayoutCreateInfo-descriptorType-03022(ERROR / SPEC): msgNum: -658548338 - Validation Error: [ VUID-VkPipelineLayoutCreateInfo-descriptorType-03022 ] | MessageID = 0xd8bf598e | vkCreatePipelineLayout():  max per-stage sampler bindings count (16539) exceeds device maxPerStageDescriptorUpdateAfterBindSamplers limit (1024). The Vulkan spec states: The total number of descriptors with a descriptorType of VK_DESCRIPTOR_TYPE_SAMPLER and VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER accessible to any given shader stage across all elements of pSetLayouts must be less than or equal to VkPhysicalDeviceDescriptorIndexingProperties::maxPerStageDescriptorUpdateAfterBindSamplers (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-VkPipelineLayoutCreateInfo-descriptorType-03022)
    Objects: 0
VUID-VkPipelineLayoutCreateInfo-pSetLayouts-03036(ERROR / SPEC): msgNum: -662943472 - Validation Error: [ VUID-VkPipelineLayoutCreateInfo-pSetLayouts-03036 ] | MessageID = 0xd87c4910 | vkCreatePipelineLayout():  sum of sampler bindings among all stages (16539) exceeds device maxDescriptorSetUpdateAfterBindSamplers limit (1024). The Vulkan spec states: The total number of descriptors of the type VK_DESCRIPTOR_TYPE_SAMPLER and VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER accessible across all shader stages and across all elements of pSetLayouts must be less than or equal to VkPhysicalDeviceDescriptorIndexingProperties::maxDescriptorSetUpdateAfterBindSamplers (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-VkPipelineLayoutCreateInfo-pSetLayouts-03036)

billhollings commented 2 weeks ago

Please run vkDoom with the following environment variables set:

MVK_CONFIG_DEBUG=1
MVK_CONFIG_LOG_LEVEL=4
MVK_CONFIG_USE_METAL_ARGUMENT_BUFFERS=1

This will log the shaders, descriptor sets, and errors.

Please ZIP up that log into a file, and post it here.

Also, to try to replicate this here, we'll need help getting vkDoom set up here to test it.

A Google search leads to the following repos:

https://github.com/dpjudas/VkDoom https://github.com/ZDoom/zdoom-macos-deps

Is this correct? Or is vkDoom found somewhere else?

Is a macOS prebuilt app somewhere? The VkDoom repo mentions nightly builds, but there are no macOS binaries archived there.

If these are the correct repos, we'll need some more detail on how to work with this:

The deps repo talks about identifying targets for the build, but it's not clear (even from the build.py help text) which targets need to be built.
What should be done with the built deps from the deps repo? How do they integrate into the VkDoom repo when it is built?
After it is built, how do we run VkDoom?
At runtime, where in VkDoom is the error identified in this issue occurring? How can the error be replicated here?

DUOLabs333 commented 2 weeks ago

Once you have cloned zdoom-macos-deps, you should be able to build with ./build.py --target vkdoom. This requires a lot of space (~3 GB), and it takes some time. This should build vkdoom (ie, cloning vkDoom separately shouldn't be required).
There are (unfortunately) no prebuilt binaries for macOS available (maybe you should be able to run the Windows binaries under Wine, but I haven't tried this, and may introduce a confounding variable).
The error prevents vkDoom from starting, so at runtime, just before the title screen loads, you should get an error message about a pipeline shader failing to compile.

billhollings commented 2 weeks ago

Thanks. I've got it built now, but hitting:

Cannot find a game IWAD (doom.wad, doom2.wad, heretic.wad, etc.).

Did I miss an install step? I can't find a WAD in the repo dir. Where can we get one?

DUOLabs333 commented 2 weeks ago

Oh, right forgot about that --- usually you can get them off of the Internet Archive, but they're down right now. I'm not at my computer right now, so I'll send the WAD to you when I'm in front of it.

billhollings commented 2 weeks ago

NM. I sourced on online. I'm able to replicate the error now.

DUOLabs333 commented 2 weeks ago

One thing I have noticed is that on 1.2.9, the structs in the converted MSL used packed_floats, while on 1.2.11, it's just float.

billhollings commented 2 weeks ago

Okay. I believe I see what the issue is now.

The app is using the following descriptor set, containing a runtime array of up to 16,536 combined image samplers:

[mvk-debug] Created VkDescriptorSetLayout 0x600002aa8e40 with 1 bindings:
    0: VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER     with up to 16536 elements at binding 0

With MVK_CONFIG_USE_METAL_ARGUMENT_BUFFERS=0, Metal is choking on the declarations of an arrays of 16,536 textures and samplers, when the maximum values without Metal argument buffers are 128 and `16, respectively.

With MVK_CONFIG_USE_METAL_ARGUMENT_BUFFERS=1, SPIRV-Cross and MoltenVK need to recast the variable-length array into two variable-length arrays within the argument buffer, one for textures and one for samplers. It's not possible to have two variable-length arrays in one structure.

Until recently, MoltenVK allocated the entire maximum count, which in your case, effectively created two separate 16,536 arrays in the Metal argument buffer. This was changed in PR #2273, to reduce memory and avoid pool exhaustion, as identified in issue #2246.

I'll investigate some options for dealing with this.

DUOLabs333 commented 2 weeks ago

I think I saw an issue like this earlier (I forgot whether it was filed with MoltenVK or SPIRV-Cross) where unless you actually access a vector in the SPIR-V code, the corresponding vector in the outputted MSL will have a length of 1. I'm not sure they are related though.

billhollings commented 2 weeks ago

the corresponding vector in the outputted MSL will have a length of 1

MoltenVK deliberately sets the array length to 1 in MSL for any variable-length arrays.

This is because the array length is not known at shader conversion and compile time. Setting it in MSL to the maximum size it can have, like your 16,536, results in a Metal validation error when the descriptor set only contains a smaller count.

This is not the problem here. The problem here is that VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER results in two variable-length arrays in, which is not possible.

The likely solution is to change it from two arrays, to an array of two-member structures. This requires work both in SPIRV-Cross and MoltenVK.

DUOLabs333 commented 2 weeks ago

Could you have two member structs, each with 1 VLA?

billhollings commented 2 weeks ago

Could you have two member structs, each with 1 VLA?

Each Metal argument buffer is just a set of resource slots. Basically just a large array, very similar to a Vulkan descriptor set.

If we have fixed length arrays in a Metal Argument Buffer:

MetalArgBuff {
    textures[10];
    samplers[10];
}

the shader knows where the offset is for the first sampler.

If, instead, we have two variable length arrays:

MetalArgBuff {
    textures[];
    samplers[];
}

the shader doesn't know how to offset into the first sampler, because it doesn't know how far the textures array will extend.

This is why there can be only be one variable length array in each descriptor set, and why it has to be at the end. This is true of both Metal and Vulkan.

I believe you are suggesting that we wrap each of textures[] and samplers[] in its own struct. If so, I don't see how that will help the shader know where to offset into the overall argument buffer to find the first sampler.

This is why I think the way around this is to declare an array of two-element structs:

ImgSamp {
    texture;
    sampler;
}

MetalArgBuff {
    ImgSamp imgSamps[];
}

The shader can then find any sampler (or texture) in the Metal arg buffer, by indexing into the array and then offsetting within the struct to get either member.

DUOLabs333 commented 2 weeks ago

Yeah, that makes sense.

DUOLabs333 commented 1 week ago

In the meantime, I worked around the issue by disabling variable descriptor sets (hopefully, this is only temporary).

KhronosGroup / MoltenVK

MoltenVK 1.2.11 breaks vkdoom #2374