[SPIRV] Add -fspv-flatten-resource-array-bindings-only - #8705
[SPIRV] Add -fspv-flatten-resource-array-bindings-only#8705Youssef Abusamra (l3rp) wants to merge 2 commits into
Conversation
- Reserves 1 binding slot per array element for resource arrays matching -fspv-flatten-resource-arrays but without emitting separate SPIRV variables/bindings. - Adds CodeGenSPIRV tests covering the non optimized and -O3 cases.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
@microsoft-github-policy-service agree |
Gregory Roth (pow2clk)
left a comment
Gregory Roth (pow2clk)
left a comment
There was a problem hiding this comment.
My main confusion here is how the previous -fspv-flatten-resource-arrays flag was useful. The way the commit message for #2397 reads, it sounds like it's trying to solve this exact problem, but I don't see why flattening the arrays was necessary. Perhaps the concern was without individual variables, they might be consolidated to fill in the gaps? Is there anywhere that might happen?
The only way this is relevant to your change is if the actual solution is to remove the old behavior and replace it with this.
| array size) instead of splitting it into one variable per element. Use this | ||
| when you want later-declared resources to avoid overlapping the numbers an | ||
| array would need under DX, without requiring the array itself to be | ||
| split up or unrolled. |
There was a problem hiding this comment.
I hate to bikeshed over naming, but I don't think what this does can be described as flattening even though it shares some behavior with the flag that does.
Perhaps something like spv-dx-resource-array-binding? I'm open to other suggestions.
There was a problem hiding this comment.
-fspv-flatten-resource-arrays is for when non uniform resource accesses are unsupported AFAIK, an OpSwitch with an array element per case is generated when a non compile time constant is used.
I think -fspv-dx-resource-array-binding would be less "obvious" though?
|
There was a problem hiding this comment.
Pull request overview
Adds a SPIR-V option that reserves binding slots for resource-array elements without splitting arrays into separate variables.
Changes:
- Adds and parses the new compiler option.
- Extends SPIR-V binding allocation.
- Adds documentation, release notes, and optimized/unoptimized tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
include/dxc/Support/HLSLOptions.td |
Defines the command-line option. |
include/dxc/Support/SPIRVOptions.h |
Stores the option state. |
lib/DxcSupport/HLSLOptions.cpp |
Parses and validates option availability. |
tools/clang/lib/SPIRV/DeclResultIdMapper.cpp |
Reserves array-sized binding ranges. |
tools/clang/test/CodeGenSPIRV/vk.binding.cl.flatten-array-bindings-only.example1.hlsl |
Tests unoptimized binding allocation. |
tools/clang/test/CodeGenSPIRV/vk.binding.cl.flatten-array-bindings-only.example1-optimized.hlsl |
Tests optimized output without array splitting. |
docs/SPIR-V.rst |
Documents behavior and usage. |
docs/ReleaseNotes.md |
Announces the option. |
Suppressed comments (1)
tools/clang/test/CodeGenSPIRV/vk.binding.cl.flatten-array-bindings-only.example1-optimized.hlsl:18
[unroll]is attached to themaindeclaration here, but this attribute only applies to loop statements, so the compiler diagnoses/ignores it and the loop is not explicitly unrolled. Move it immediately before theforstatement.
[unroll]
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // CHECK: OpDecorate %NextTexture Binding 6 | ||
| // CHECK: OpDecorate %MySamplers Binding 7 | ||
| Texture2D MyTextures[5] : register(t0); | ||
| Texture2D NextTexture; // This is suppose to be t6. |
| // CHECK-NOT: MyTextures_0_ | ||
| // CHECK-NOT: MySamplers_0_ | ||
| Texture2D MyTextures[5] : register(t0); | ||
| Texture2D NextTexture; // This is suppose to be t6. |
-fspv-flatten-resource-arraysbut without emitting separate SPIRV variables/bindings.