MSL: Fix regression error in argument buffer runtime arrays. #2222
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Argument buffers can contain multiple runtime arrays if they have fixed lengths as specified by the binding API. Regression error had assumed each runtime array is in separate argument buffer with undefined array length. - Add CompilerMSL::is_var_runtime_size_array() to include test for setting of array length via CompilerMSL::add_msl_resource_binding(). - Fixed unrelated test case MSL compile syntax failure when acceleration structure is the first entry point function argument (unrelated).
billhollings
force-pushed
the
fix-runtime-array-regression
branch
from
d06fb45
to
4a42191
Compare
cdavis5e
approved these changes
Nov 3, 2023
| fragment void main0(, raytracing::acceleration_structure<raytracing::instancing> topLevelAS [[buffer(0)]]) | ||
| fragment void main0(raytracing::acceleration_structure<raytracing::instancing> topLevelAS [[buffer(0)]]) |
There was a problem hiding this comment.
...How did this even pass before??
There was a problem hiding this comment.
It don't think it ever did pass Metal compile. The shaders were new when added in da9c861, so no regression check, and I assume it was committed without an actual unit test run on macOS. Unit tests on other OS's bypass the Metal compile step, of course.
SPIRV-Cross Github CI doesn't build and test on macOS. Perhaps we should add a CI test sequence for that?
There was a problem hiding this comment.
SPIRV-Cross Github CI definitely runs on macOS. Did that break?
There was a problem hiding this comment.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Argument buffers can contain multiple runtime arrays if they have fixed lengths as specified by the binding API. Regression error had assumed each runtime array is in separate argument buffer with undefined array length.
Add
CompilerMSL::is_var_runtime_size_array()to include test for setting of array length viaCompilerMSL::add_msl_resource_binding().Fixed unrelated test case MSL compile syntax failure when acceleration structure is the first entry point function argument (unrelated).
This has been tested in MoltenVK, but I couldn't think of a way of defining a SPRIV-Cross unit test for this, as it would involve calling
add_msl_resource_binding()with some complex input.