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: Calculate argument buffer allocation size taking variable counts into account. #2199

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

Conversation

js6i
Copy link
Collaborator

@js6i js6i commented Apr 3, 2024

When allocating descriptors from the pool, we should only request as much as needed for the variable number of descriptors, if applicable.

It looks slightly dicey to use the fixed size argument encoder when encoding variable sized bindings. Since here we're creating the encoder to calculate the size, maybe we could use it for encoding too..

Copy link
Contributor

@billhollings billhollings left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this.

I've requested a couple of changes.

It looks slightly dicey to use the fixed size argument encoder when encoding variable sized bindings. Since here we're creating the encoder to calculate the size, maybe we could use it for encoding too..

Good point. And good catch.

Having each descriptor set carry its own encoder sounds like a lot. Perhaps we could have the descriptor set defer to the layout's encoder, unless the variable descriptor count is less than the descriptor count of the variable-length binding.

However, we are going to move away from argument encoders very soon. So, I don't think we should bother making things that complex unless we actually determine it is causing a problem.

MoltenVK/MoltenVK/GPUObjects/MVKDescriptor.mm Outdated Show resolved Hide resolved
MoltenVK/MoltenVK/GPUObjects/MVKDescriptorSet.mm Outdated Show resolved Hide resolved
@js6i js6i force-pushed the variable-counts branch 3 times, most recently from efba837 to 388c31a Compare April 4, 2024 08:15
@js6i
Copy link
Collaborator Author

js6i commented Apr 4, 2024

Having each descriptor set carry its own encoder sounds like a lot. Perhaps we could have the descriptor set defer to the layout's encoder, unless the variable descriptor count is less than the descriptor count of the variable-length binding.

I think we could do that. Are they expensive though? I would have thought it's fine to create them on demand, for single use.

However, we are going to move away from argument encoders very soon. So, I don't think we should bother making things that complex unless we actually determine it is causing a problem.

It's causing a validation error. That said, there's also another one, when the shader has the maximum size array declared, but the buffer that gets bound is smaller. Let's hold off merging for now, I'll see what can be done about the other one.

@js6i
Copy link
Collaborator Author

js6i commented Apr 10, 2024

I updated the PR with commits that fix the validation errors. However now, in a number of tests, I'm getting GPU crashes instead - which seem to be triggered by having flexible arrays in the shaders. Marking as WIP..

@js6i js6i marked this pull request as draft April 10, 2024 16:18
@billhollings billhollings changed the title Calculate argument buffer allocation size taking variable counts into account. WIP: Calculate argument buffer allocation size taking variable counts into account. Apr 15, 2024
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

2 participants