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] HLSL style register assignment for resource arrays #6932

Conversation

manas-kulkarni
Copy link

@manas-kulkarni manas-kulkarni commented Sep 25, 2024

  • New spirv flag -fspv-hlsl-resource-arrays
  • When the flag is specified, resource arrays will use one binding per array element
    • We need this change to support array textures in our engine
  • Matches with hlsl style register where arrays take up one register per array element
  • Added test shader for the clang tests
Texture2D MyTextures[5];
Texture2D AnotherTexture;

MyTextures will take 5 bindings, AnotherTexture will be at binding 5

Closes #6927

- Array bindings that use implicit register assignment take one binding number per array element
@manas-kulkarni manas-kulkarni requested a review from a team as a code owner September 25, 2024 07:11
Copy link
Contributor

github-actions bot commented Sep 25, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@manas-kulkarni
Copy link
Author

@microsoft-github-policy-service agree

Copy link
Collaborator

@s-perron s-perron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it is adding a new feature to DXC, and not a fix to an existing feature. We may need to discuss this more.

The first issue is that I'm concerned this does not do what you think it does.

Second, I want to make sure a solution does not already exist for what you want to do. Without knowing you exact workflow, I do not know what to suggest. Here is my guess.

Some developers use -fspv-flatten-resource-arrays to get resource arrays where each element actually has a different binding. If that is want is important you, that is how you do it. I believe this came from a time when descriptor indexing was not commonly available.

Other use explicit binding, so they know where each resource will be placed.

The last possibility is to use spir-v reflect. This is what I think you want to do. I'm totally guessing, but I'm guessing you use implicit binding for DX. You get the register numbers from reflection, and you want reusing the same numbers for the Vulkan bindings. If that is the case, I would suggest not using the register numbers from the DXIL, but to run the SPIR-V through spir-reflect to get the binding numbers that are Vulkan specific. The binding is different enough that you will keep running into problem using the DXIL register numbers. For example, in Vulkan we have to have a different binding for the counter buffers that correspond to RWStructuredBuffers. You cannot match the dxil binding numbers in these cases, so you will still have to do reflection on the SPIR-V.

Let me know you work flow, and we can try to figure out a solution.

@@ -390,6 +390,8 @@ def fspv_target_env_EQ : Joined<["-"], "fspv-target-env=">, Group<spirv_Group>,
HelpText<"Specify the target environment: vulkan1.0 (default), vulkan1.1, vulkan1.1spirv1.4, vulkan1.2, vulkan1.3, or universal1.5">;
def fspv_flatten_resource_arrays: Flag<["-"], "fspv-flatten-resource-arrays">, Group<spirv_Group>, Flags<[CoreOption, DriverOption]>,
HelpText<"Flatten arrays of resources so each array element takes one binding number">;
def fspv_hlsl_resource_arrays: Flag<["-"], "fspv-hlsl-resource-arrays">, Group<spirv_Group>, Flags<[CoreOption, DriverOption]>,
HelpText<"Resource arrays use one binding per array element to match hlsl style">;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not an accurate description of what is happening. In Vulkan, an array of resources take just a single binding. In the test below, MyTextures[5] will still use a single binding. And then binding 1-4 are left unused.

Also the name of option is not very descriptive. What is an HLSL resource array? You should reference something in DirectX, you seem to want to try to match DX's binding model in someway. Even that I believe is very difficult because Vulkan's binding model is very different.

@manas-kulkarni
Copy link
Author

manas-kulkarni commented Sep 26, 2024

We were able to get same functionality with -fspv-flatten-resource-arrays and -fcgl. Passing -fcgl skips the pass where separate resources are created per array element

@manas-kulkarni manas-kulkarni deleted the fspv_implicit_hlsl_resource_arrays branch September 26, 2024 13:38
@manas-kulkarni
Copy link
Author

manas-kulkarni commented Sep 26, 2024

So if the -fcgl in future also creates separate resources per array element, we would need this separate flag though. In current version with -fspv-flatten-resource-arrays + -fcgl, only the binding numbers are used per array element and the resources themselves stay as arrays.
If this behavior change in future, we would need the -fspv-dx/hlsl-resource-arrays option

@s-perron
Copy link
Collaborator

So if the -fcgl in future also creates separate resources per array element, we would need this separate flag though. In current version with -fspv-flatten-resource-arrays + -fcgl, only the binding numbers are used per array element and the resources themselves stay as arrays.
If this behavior change in future, we would need the -fspv-dx/hlsl-resource-arrays option

Fcgl turns off all optimizations so that we can test what the spirv is before spirv-opt does anything. You may need to run the optimizer separately to generate valid Spir-V.

@manas-kulkarni
Copy link
Author

Yes. We run spirv-opt in separate pass. Were wondering if the behavior of Fcgl will ever change and it does include the spirv passes that create new variables per array element, we would need a different flag to perform the current behavior of fspv-flatten-resource-arrays + fcgl

@s-perron
Copy link
Collaborator

-fcgl will not change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Feature Request][SPIR-V] HLSL style binding assignment for array of resources
2 participants