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] Support Texture2D<half> in universal SPIR-V #6987

Open
forbiddencactus opened this issue Oct 28, 2024 · 3 comments
Open

[SPIR-V] Support Texture2D<half> in universal SPIR-V #6987

forbiddencactus opened this issue Oct 28, 2024 · 3 comments
Labels
enhancement Feature suggestion spirv Work related to SPIR-V

Comments

@forbiddencactus
Copy link

forbiddencactus commented Oct 28, 2024

Description
Before the following change to DXC, code sampling textures of <half> size compiled but then failed SPIR-V validation because it ran afoul of a Vulkan rule. This was non optimal, but acceptable behaviour, because it could be worked around in SPIR-V postprocessing or by running it through a custom validator, particularly in cases where SPIR-V is just an intermediate step, like compiling iOS shaders.

The new behaviour is harder to work around since it's an error in DXC itself.

Better behaviour would be if the HLSL compiled and just generated an OpTypeImage of 32 bits, or if this particular error could be disabled via a compiler flag, so it can be manually postprocessed / worked around. As it is, it's a problem, because valid HLSL that compiles on DXIL platforms suddenly stops compiling on SPIR-V platforms.

Steps to Reproduce
Compile HLSL code sampling a Texture2D<half> texture with -enable-16bit-types flag.

Environment

  • DXC version 2024-07-31
  • Host Operating System Windows
@forbiddencactus forbiddencactus added bug Bug, regression, crash needs-triage Awaiting triage spirv Work related to SPIR-V labels Oct 28, 2024
@Keenuts
Copy link
Collaborator

Keenuts commented Oct 28, 2024

I'd be temped to close this as working as intended. (@s-perron for a second opinion).
The use-case I understand is: HLSL -> SPIR-V -> Metal
Metal supports 16-bit textures, Vulkan does not. Hence, DXC does not support those for SPIR-V.

The fact that the SPIR-V backend refuses to compile valid HLSL code happens often, because Vulkan is not DirectX. And the behavior we have today matches what we can do with Vulkan.

I don't think adding a flag to generate invalid code just for this specific purpose is a good solution.
I see another alternative:

  • pre-process the HLSL, upgrading half textures to 32-bit textures, but annotate those with the RelaxedPrecision decoration.
  • compile HLSL
  • post-process your SPIR-V to transform textures with this decoration to 16-bit textures.

@Keenuts Keenuts added usability Issues impacting usability and removed bug Bug, regression, crash needs-triage Awaiting triage labels Oct 28, 2024
@Keenuts Keenuts moved this to For Google in HLSL Triage Oct 28, 2024
@s-perron
Copy link
Collaborator

I disagree. No link was added, but the new error is not a more meaningful error. It is an internal compiler error, and should be fixed. Also, You can target the "universal1.5" target env, which should not care about Vulkan specific restrictions, even though we are probably not consistent about that.

Old working link: https://godbolt.org/z/oGnsWTxa5
New failing link: https://godbolt.org/z/hqTqdW8cK

With that said I do not know when we will get to fixing it. I'll tentively schedule for the next-next release. If you need a fix sooner, you can try to submit a fix that we will review.

@s-perron s-perron added this to the Next+1 Release milestone Oct 28, 2024
@Keenuts Keenuts added needs-triage Awaiting triage and removed needs-triage Awaiting triage labels Oct 28, 2024
@s-perron s-perron moved this from For Google to Triaged in HLSL Triage Oct 28, 2024
@Keenuts
Copy link
Collaborator

Keenuts commented Oct 29, 2024

I disagree. No link was added, but the new error is not a more meaningful error. It is an internal compiler error

This doesn't always happen. We also do emit a diag for some paths. But I agree, the assert we hit in your example is not expected, and this should be fixed independently from this question. Opening #6989 to track this specific crash.

You can target the "universal1.5" target env, which should not care about Vulkan specific restrictions, even though we are probably not consistent about that.

Ah forgot that we support this. Then yes, we should allow emission of such textures in that case. Renaming this as a feature request.

@Keenuts Keenuts added enhancement Feature suggestion and removed usability Issues impacting usability labels Oct 29, 2024
@Keenuts Keenuts changed the title [SPIR-V] Correct HLSL Texture2D<half> does not compile on SPIR-V pathway [SPIR-V] Support Texture2D<half> in universal SPIR-V Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature suggestion spirv Work related to SPIR-V
Projects
Status: New
Status: Triaged
Development

No branches or pull requests

3 participants