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

WIP: Let SPIRV-Cross assign MSL resource indices. #1173

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

js6i
Copy link
Collaborator

@js6i js6i commented Dec 4, 2020

There's an issue with assigning Metal resource indices upfront that this PR attempts to address.
Suppose that a client creates a pipeline with more resources in the layout than is allowed for a shader to use, say, more than 16 samplers. In the current scheme, even if no single stage uses too many samplers by itself, they will get numbered globally for the entire pipeline and crash when an index is > 15.
The proposed solution is to let SPIRV-Cross allocate the resource indices on demand. There's already a mechanism to do that, although I made some changes to make use of it more easily.
Apart from fixing that problem, there are some other upsides to it: we can stop binding unused resources and maybe handle implicit buffers more nicely.
Currently this PR is in a work in progress, not quite working yet state. Although it does seem to work with some games I was testing. @billhollings please let me know if that's a direction you like, or should the problem be addressed in some other way?

@billhollings billhollings changed the title Let SPIRV-Cross assign MSL resource indices. WIP: Let SPIRV-Cross assign MSL resource indices. Dec 4, 2020
@billhollings
Copy link
Contributor

billhollings commented Dec 4, 2020

@js6i Thanks for the ideas (and work on this).

I and @cdavis5e can review this in more detail after the SDK release is out next week. Ideas like this might also help when native MSL is submitted too.

I'm also currently working on argument buffers, which also won't make it into this SDK release, unfortunately. That will also impact how descriptor sets are handled.

@oscarbg
Copy link

oscarbg commented Dec 11, 2020

@js6i can share with what games are you testing this patch? just curious..

@js6i
Copy link
Collaborator Author

js6i commented Dec 11, 2020

Not many so far, I tried Hitman 2, Risk of Rain 2 and Wolfenstein II. Of which the latter has this problem.

@oscarbg
Copy link

oscarbg commented Feb 13, 2021

@js6i any update on this patch? seems like it not will ready for next week "freeze" for next Vulkan SDK release, right?

@js6i
Copy link
Collaborator Author

js6i commented Feb 15, 2021

No, it on pause until the argument buffer situation is clear.

@oscarbg
Copy link

oscarbg commented May 12, 2021

No, it on pause until the argument buffer situation is clear.

now that support is in VK SDK 1.2.176 (at least enabled on Big Sur, and Catalina Intel GPU only), can share some update?
thanks..

@js6i
Copy link
Collaborator Author

js6i commented May 18, 2021

There's no updates yet. That said, as Intel GPU's are not that relevant for graphically advanced games, if support on other GPU's is blocked by something longer term, there's still motivation for these changes. Otherwise I'm not sure there is as, if I remember correctly, the resource limits are significantly raised when argument buffers are used.

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

Successfully merging this pull request may close these issues.

None yet

3 participants