MVKShaderLibrary: Handle specializtion with macros (v2) #2441
+260
−27
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
|
|
dboyan
force-pushed
the
spec-macro-fix-v2
branch
from
ffdccab to
154a7ee
Compare
as a part of SPIRVToMSLConversionResultInfo, to be used later when processing specialization constants as macro need special handling at compile time vs. function constant.
dboyan
force-pushed
the
spec-macro-fix-v2
branch
from
154a7ee to
50244e6
Compare
The converted MSL may use macro instead of function constants to realize spirv specialization constant for various reasons (e.g. when the constant is used as array size). In this case, we should define the macros at shader compilation stage and generate different variants of the metal shader library depending on macro-value mapping to make specializtion work properly when we cannot rely on metal's specialization. We use the information from SPIRV-Cross to decide the usage of macros and store variants of MTLLibrary according to macro values.
dboyan
force-pushed
the
spec-macro-fix-v2
branch
from
50244e6 to
bfb76a6
Compare
billhollings
approved these changes
Feb 17, 2025
|
Yes. I think so. |
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.
The converted MSL may use macro instead of function constants to realize spirv specialization constant for various reasons (e.g. when the constant is used as array size).
In this case, we should define the macros at shader compilation stage and generate different variants of the metal shader library depending on macro-value mapping to make specialization work properly when we cannot rely on metal's specialization.
There have been multiple use cases that require the proper handling we are missing, including llama.cpp and Vulkan CTS tests on
VK_KHR_zero_initialize_workgroup_memory(we haven't supported it yet but I'm working on it).This PR supersedes #2434, and is intended to be a version that can be mainlined. It is almost functionally equivalent to the previous PR (with some correctness fixes), but uses a new API from SPIRV-Cross to obtain the information of macros instead of unreliable heuristics to guess from the translated shader code. The new SPIRV-Cross API has been merged (KhronosGroup/SPIRV-Cross#2442).
There is one caveat though. Currently I assume the specialization constants (that are turned into macros) are 32-bit integers. It works on all uses I have seen because they can only be sizes of arrays, which are integers and usually represented with 32-bit values. I'm not very sure if my handling is enough or the most appropriate approach here.The current version is taking types into account.
Fixes: #2423