Skip to content

[SPIRV] Add -fspv-flatten-resource-array-bindings-only - #8705

Open
Youssef Abusamra (l3rp) wants to merge 1 commit into
microsoft:mainfrom
l3rp:spv-array-flatten-binding-only
Open

[SPIRV] Add -fspv-flatten-resource-array-bindings-only#8705
Youssef Abusamra (l3rp) wants to merge 1 commit into
microsoft:mainfrom
l3rp:spv-array-flatten-binding-only

Conversation

@l3rp

Copy link
Copy Markdown
  • 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.
  • The motive is to have identical bindings to the DX path for a shared binding system.

- 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

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@l3rp

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

@pow2clk Gregory Roth (pow2clk) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Comment thread docs/SPIR-V.rst
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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

-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?

@damyanp

Copy link
Copy Markdown
Member

[Auto-generated note from Damyan Pepper (@damyanp)]

This looks like a user-visible bug fix/feature change. Please add (or point to) the corresponding entry in docs/ReleaseNotes.md.

If release-note coverage is planned in a related PR (including one that hasn’t been submitted yet), please mention that plan/link so we can avoid duplicate notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

3 participants