Skip to content
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

[SPIR-V] RWTexture2D without format qualifier should emit Unknown type for its OpTypeImage #7047

Closed
nicebyte opened this issue Jan 4, 2025 · 4 comments
Labels
bug Bug, regression, crash needs-triage Awaiting triage spirv Work related to SPIR-V

Comments

@nicebyte
Copy link
Contributor

nicebyte commented Jan 4, 2025

Description
Consider the following simple compute shader:

[[vk::binding(0, 0)]] RWTexture2D<float4> outputImage;

[numthreads(4, 4, 1)] void CSMain(uint3 tid
                                  : SV_DispatchThreadID) {
  outputImage[tid.xy] = float4(sin(tid.x),cos(tid.y),0.0,1.0);
}

this produces SPIR-V with the following OpTypeImage corresponding to outputImage:

%type_2d_image = OpTypeImage %float 2D 2 0 0 2 Rgba32f

Note that the format is Rgba32f. Most GPUs that support WriteWithoutFormat capability seem to ignore the format parameter of OpTypeImage anyway, so this doesn't really affect behavior on them. For GPUs that actually do care about the format somehow, this would corrupt the image if e.g. an RGBA8 image is bound inadvertently (and there seems to be no validation warning, I filed a separate issue for that).

In any case, the generated code seems inconsistent with the implied HLSL semantics: as far as im aware, D3D hlsl assumes StorageImageWriteWithoutFormat capability by default,. To match this in SPIR-V, the corresponding OpCapability should be emitted and Unknown format used, unless an explicit image_format qualifier is used.

Environment

  • DXC version: trunk
  • Host Operating System: Windows
@nicebyte nicebyte added bug Bug, regression, crash needs-triage Awaiting triage spirv Work related to SPIR-V labels Jan 4, 2025
@nicebyte
Copy link
Contributor Author

nicebyte commented Jan 4, 2025

IMO this needs fixed because this behavior can cause shader authors to implicitly rely on ReadWithoutFormat/WriteWithoutFormat capability without having it explicitly declared in the SPIR-V. If the required OpCapability were included, VVL would complain and make the shader author aware they're relying on it.

@Keenuts
Copy link
Collaborator

Keenuts commented Jan 6, 2025

Hi,

This issue has been raised multiple times (#4941 is the last one).

this produces SPIR-V with the following OpTypeImage corresponding to outputImage:
%type_2d_image = OpTypeImage %float 2D 2 0 0 2 Rgba32f

I did suggested to remove the format guess in the past, but a good argument was raised against: when using the same shaders across multiple APIs, this format guessing can be useful for both reflection, and execution.
In addition, it looks like 19% of the devices in gpuinfo don't support this, and some are not that old (12 devices reported no read support in 2024)
If you still want to have unknown as a format, you can use [[vk::image_format("unknown")]].

IMO this needs fixed because this behavior can cause shader authors to implicitly rely on ReadWithoutFormat/WriteWithoutFormat capability without having it explicitly declared in the SPIR-V.

Not sure I understand. Do you have a case where the capability is not added when it should?
Normally this capability is handled at a late stage, so as soon as Unknown is used in a format, the capability is added.

@nicebyte
Copy link
Contributor Author

nicebyte commented Jan 6, 2025

Not sure I understand. Do you have a case where the capability is not added when it should? Normally this capability is handled at a late stage, so as soon as Unknown is used in a format, the capability is added.

Sorry, I should have been more clear. What I had in mind was the following scenario:

  1. The developer initializes Vulkan without explicitly requesting write-without-format support
  2. The developer writes (or copies over) HLSL shader that omits the explicit format qualifier on a storage image descriptor.
  3. DXC helpfully guesses the format for OpTypeImage and writes e.g. Rgba32f to SPIR-V
  4. The developer binds an image with a different format (e.g. RGBA8).
  5. The developer's GPU/driver configuration has support for write-without-format, it happens to simply ignore the format parameter of OpTypeImage and does its thing. Everything happens to work and the developer is none the wiser.
  6. When the software is run on a system that does care about format, the image is corrupted because the format of the bound image and the format expected by the shader dont match (e.g. [Intel GPU][Linux] Severe artifacts with "Light Blooms" and "Compute Shaders" enabled ValveSoftware/Dota-2-Vulkan#314, afaiu this exact bug is what eventually prompted the appearance of the vk::image_format qualifier in HLSL).

In the scenario above, the developer has come to rely on write-without-format without even requesting it or knowing about it.

In addition, it looks like 19% of the devices in gpuinfo don't support this, and some are not that old (12 devices reported no read support in 2024)

IMO, this could also be viewed as a strong argument in favor of NOT guessing the format and just writing Unknown when an explicit qualifier is missing, because of the exact scenario outlined above. If a developer wishes to target such devices, they need to provide an explicit format qualifier, because whatever dxc guesses is quite likely to not match the format of the actual bound image view.

Granted, VVL need to issue an error or warning in this case, which they currently don't (KhronosGroup/Vulkan-ValidationLayers#9129). But even if they did, the compiler not guessing in the first place would make diagnostics easier - the developer would immediately and directly be made aware that they're relying on a feature they didn't enable (rather than in a round-about way, by a "mismatched format" error).

, but a good argument was raised against: when using the same shaders across multiple APIs, this format guessing can be useful for both reflection, and execution.

I would object that when writing shaders across multiple APIs in HLSL, I would expect the compiler to try to maintain consistent semantics across different targets. If in D3D HLSL, read/write-without-format is implied, then it should be also implied for SPIR-V HLSL unless the explicit qualifier is provided. Otherwise, we'd find ourselves in a situation where the same exact code means two very different things on different platforms, and what's the point of cross-platform shaders then?

I understand the tooling argument, but it feels wrong to bend the meaning of generated code out of shape just because it happens to help some tool. Maybe putting the guessed format under some sort of different decoration is the answer here.

Ultimately you decide whether the change is worth it, hope you consider my points above when you do :)

@s-perron
Copy link
Collaborator

s-perron commented Jan 7, 2025

We will not be doing this in DXC. It is a change of behaviour that will force existing code to have to change if the developer wants to target a platform that does not support the extension.

In Clang, the current unofficial plan is to use unknown when targeting Vulkan 1.3, which guarentees the capabilities are available. For earlier versions of Vulkan, we will need to replicate the DXC behaviour for backwards compatibility.

@s-perron s-perron closed this as completed Jan 7, 2025
@damyanp damyanp moved this to Triaged in HLSL Triage Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug, regression, crash needs-triage Awaiting triage spirv Work related to SPIR-V
Projects
Status: Triaged
Development

No branches or pull requests

3 participants