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

Apply SPIRV-Cross workarounds for Metal bugs #2248

Merged

Conversation

aitor-lunarg
Copy link
Collaborator

2 workarounds introduced:

  • Fragments with side effects will now execute following Vulkan
  • Depth/Stencil write postponed to post fragment execution when input attachment and depth/stencil attachment reference the same resource

Fixes #2232
Fixes #2233

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.

Nice work!

I've made a suggestion for handing the dynamic-rendering case.

Plus some nit-picking on variable name styles.

Looks like the corresponding SPIRV-Cross patch is already rolled into MoltenVK? Is that correct?

MoltenVK/MoltenVK/GPUObjects/MVKPipeline.h Outdated Show resolved Hide resolved
MoltenVK/MoltenVK/GPUObjects/MVKPipeline.mm Show resolved Hide resolved
MoltenVK/MoltenVK/GPUObjects/MVKRenderPass.mm Outdated Show resolved Hide resolved
@aitor-lunarg
Copy link
Collaborator Author

Looks like the corresponding SPIRV-Cross patch is already rolled into MoltenVK? Is that correct?

Yes, it was already included, that's why there's no SPIRV-Cross update needed

2 workarounds introduced:
 - Fragments with side effects will now execute following Vulkan
 - Depth/Stencil write postponed to post fragment execution when
   input attachment and depth/stencil attachment reference the
   same resource
@aitor-lunarg
Copy link
Collaborator Author

Sorry for the delay on the update. I believe all comments should have been addressed. I will provide a follow up PR to handle the dynamic rendering case, mainly because it'll require bigger changes, and wanted to keep this PR small. If there's something I missed, do let me know!

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. LGTM now.

@billhollings billhollings merged commit c6373b8 into KhronosGroup:main Jun 28, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants