-
Notifications
You must be signed in to change notification settings - Fork 147
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
Adding preliminary SpecConstant
support
#154
base: main
Are you sure you want to change the base?
Conversation
CLA may need contributers to godotengine/godot#50325. (Godot is licensed under MIT license, so maybe the agreement is compatible? Not a law expert...) |
Related issue #121 (From godot, where starting of current pull is based on) |
7d4bc8a
to
e004aa2
Compare
Used code form godot/godot as starting point. See KhronosGroup#121 Next steps: 64 bit types need 2 words as operand, `SpecConstantOp` need implementation for evaluating spec constant (spirv-cross has an implementation for uint KhronosGroup/SPIRV-Cross#1463). Also maybe composite type evaluation support? Co-Authored-By: Juan Linietsky <[email protected]>
Next step would be the constant evaluator
e004aa2
to
e1dfa44
Compare
Keeping everything in one is difficult, also there might be better ways to do this (especially with the variant types). Modified some of the evaluation code for spec-constant array size, but not finished.
`memcpy` is required for strict aliasing rule. union member offset in c++ requires c++14 to guarantee (though c style code should always work on reasonable compilers.)
There may be better ways to evaluate the values (e.g. using some jit compiler that compiles to cpu native code), not sure if all the switch is easily maintainable (and if if it's possible through some |
Currently using the following test from spirv-cross will result in correct specialized array size.
related spirv assembly:
spirv-reflect output:
Changing constant value should work, but not tested. Still WIP. |
Vector composition and logical (boolean) ops left...
ba41960
to
21613d4
Compare
Moved much of recurring code into |
Code too lengthy, difficult to maintain...
21613d4
to
4697ff6
Compare
Operations done, but compositing and de-compositing vectors not included. 16 bit floating point quantization and kernel ops not implemented.
Scalar constants should work now. Tested with following glsl from spirv-cross:
Reflection output:
|
Next steps would be cleaning up clutter, removing dependency on parser and better evaluation. |
dd99a5d
to
f3a72fc
Compare
Test and much work is needed to remove dependency on parser. (Need to store constant expressions in a separate representation. Potentially also removing redundant evaluation)
Remove dependency on parser. Still, FindType needs the module pointer...
Implementation is now done, but the API needs clearing up. |
2b0499c
to
743085d
Compare
Hi! I just wanted to comment that after integrating it into a custom project, I find it very useful. I'm just hoping for approval and a merge so I can build on this as part of the vulkan sdk. :) |
In addition to the feedback I left:
|
Updated `SpvReflectEntryPoint` to have non-conflicting struct alignment. Formatting fix.
A merge error occurred in the last commit. Fixed in this one.
Now localsize flag is not part of entry point struct.
Address [problem](64e256b#commitcomment-120952316)
6124844
to
9af3b32
Compare
9af3b32
to
f722a55
Compare
Any update on this? I would like to play with these in some of my shaders in order to dynamically change the work group size. Thanks! |
Is there anything I can help out regarding this? :) I would really like to use spec constants via SPIRV-Reflect. I'm using shangjiaxuan's initial commit instead of the main branch, and its very useful. |
Sorry for the delay. Honestly, for my initial use case, just not barfing when specialization constants for the local group size are used would be fine. When I first tried it, it failed to parse if any spec constant syntax was involved in the variable. I had written up in full detail what happened somewhere. Eventually I would like to retrieve more information about the variable definition for the local group size dimensions so I can consume any spirv given to modify the local group size and JIT at dispatch time to fit the local group size requested, caching more frequently used compilations for reuse later. At the moment I'm just hard coding the specialization id, but I still need spirv-reflect to be able to parse the spirv in order to retrieve information about the entry point, etc. |
for (uint32_t j = 0; j < p_module->entry_point_count; ++j) { | ||
if (p_module->entry_points[j].spirv_execution_model == SpvExecutionModelKernel || | ||
p_module->entry_points[j].spirv_execution_model == SpvExecutionModelGLCompute) { | ||
p_module->_internal->entry_point_mode_flags[j] = 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this code block doing? Why is it overwriting the x value of the local size with the result_id that WorkGroupSize is held in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the code is written, specialization constants for workgroup size (aka local_size_id_n
), as per spirv 1.5 specification, the compiler have two options for generating code. One is using OpExecutionModeId
, and referencing resultid of three constant Ops. The other is using WorkGroupSize
builtin, which takes a uvec3 argument resultid and overwrites the OpExecutionMode
Op. When the code is written, glslc
and other compilers are relying on the second (builtin) mode for generating code in glsl. The second mode used to be deprecated in spv1.5, but later it seems the deprecation plan was moved to 1.6, and then deprecation got removed if I remember correctly.
Reference:
https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a reference of spv asm disassembled from glslc generated code that uses the builtin, see the sample test code included in pr:
tests/spec_constant/test_orig.spv.dis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the version of this PR I'm using is out of date, but in my case if a GLSL shader is using a spec constant, and is using the gl_WorkGroup variable, but the spec constant is not use to set the local_size, this code block still gets used and you end up with the incorrect value for local_size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the version of this PR I'm using is out of date, but in my case if a GLSL shader is using a spec constant, and is using the gl_WorkGroup variable, but the spec constant is not use to set the local_size, this code block still gets used and you end up with the incorrect value for local_size.
Please give me sample glsl code and compiler options to compile it. Or you can give me the disassembled code. This can help clarify your problem and help me locate it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm not sure I follow. In this case I want to get the values (16, 32, 1), since that is the actual local_size values. Where will those values reside when it's in this mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use spvReflectEvaluateResult
to evaluate the value of localsize.x resultid, for the current specialization constants in the evaluator (default values if you don't set them) to get the current uvec3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I can see how that'll result in me getting the correct answer. However, specialization constant's aren't used for the workgroup size in this example. The are hardcoded constants. It seems to me it should only enter into this mode if a specialization constant is actually used to set the workgroup size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also baffles me why the compiler generates the builtin instruction. The older version compiler didn't do this if I remember correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm currently compiled again GLSLang 302a663a38, so it's not the most up to date version.
Another comment, I would think the reflection should put this result_id somewhere else, and try to resolve the correct local size in to the local_size variable. Re-using 'x' like this is confusing, and in general the user of reflection just wants the results, so asking them to use spvReflectEvaluateResult seems like an un-needed step.
A comment about the ABI (not directed at @shangjiaxuan), I don't understand why it would be requested that this tool which is only 2 files, should have a locked ABI. It's not completed enough to start hampering the interface with a locked ABI at this point.
For what it's worth, I'll let you know why we need this in Godot.
|
Edited Description
Added specialization constant parsing and evaluation code (32 bit and 64 bit operands).
Features Added:
local_size_n
,local_size_n_id
inglsl
andnumthreads
inhlsl
) with support for builtinWorkGroupSize
when last checked is widely used in compilers likeglslc
and was in preparation for deprecation for a long time (since spv1.5).VkSpecializationInfo
with evaluation-internal data (user still need to allocateVkSpecializationMapEntry
).Sample Reflection
Code taken and adapted from
spirv-cross
(intests/spec_constant/test_orig.glsl
):Patch (in
tests/spec_constant/test_32bit.spv.dis.patch
):output:
Note that since the compiler uses
WorkGroupSize
builtin,OpExecutionMode
changes are overridden completely by the builtin instruction, instead of using the patched(2, 3, 4)
value fromOpExecutionMode
.Notes on ABI
Recent changes moved the parsed localsize mode flag to internal field, so this pr should be ABI-compatible with binaries built using older headers (aka from godot. Since
SpvReflectShaderModule
has new membersspecialization_constant_count
andspecialization_constants
, on-stack or struct memberSpvReflectShaderModule
s are not ABI-compatible with main reporsitory.) now.Notes on Arithmetic Operations.
As per glsl specification, glsl does not guarantee
(a/b)*b + (a%b)==a
for integer types. When usingglslc
, when last checked,a%b
yields OpSMod instead of OpSRem. This means the following signed integer operations gives:instead of
This is because remainder is defined as having same sign as dividend, while modulus is defined as having same sign as divisor. Modulus operator
%
is currently defined in c/c++ as taking remainder (same for most similar operations likefmod
).This is important since the test case required special treatment in patching the assembly as in
tests/spec_constant/test_32bit.spv.dis.patch
Original Description
Used code form godot/godot as starting point. See #121
Next steps: 64 bit types need 2 words as operand,
SpecConstantOp
need implementation for evaluating spec constant (spirv-cross has an implementation for uint KhronosGroup/SPIRV-Cross#1463). Also maybe composite type evaluation support?