-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
[Impeller] add support for specialization constants. #47432
[Impeller] add support for specialization constants. #47432
Conversation
…/engine into add_specialization_support
Some pending work, I need to verify + test that Variants are working correctly with the spec constants. |
Test added for that. |
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.
Only reviewed the docs, thanks!
@@ -0,0 +1,90 @@ | |||
# Specialization Constants | |||
|
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 would be nice to start with 1-2 sentences describing what a specialization constant is. This is my first time hearing the phrase, and I tried a Google search, and as you can see it's not clear where to start.
Maybe (assuming I understand now):
"""
A specialization constant, or a shader constant that can be set at runtime, allowing for more efficient code generation and flexibility.
"""
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.
Done!
* Improving performance, by removing branching and conditional code. | ||
* Code organization/size, by removing the number of shader source files required. | ||
|
||
These goals are interrrelated, we can always reduce the number of shaders by creating |
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.
These goals are interrrelated, we can always reduce the number of shaders by creating | |
These goals are interrelated, we can always reduce the number of shaders by creating |
|
||
## Implementation | ||
|
||
Currently the only constant values that are support are 32bit ints. This should be sufficient for now as we |
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 would re-write this paragraph (tried to do it via GH suggestions) to be more opinionated. Something like:
32-bit integers are supported as constant values and should represent:
- `true`/`false`
(example)
- select-style constants:
(example)
AVOID adding constant color values or anything more complex.
Taking a look 👀 |
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.
Warning
This patch is pretty cool.
ⓘ Note
Nits and minor questions.
// kSaturation, | ||
// kColor, | ||
// kLuminosity, | ||
// Note, this isn't a switch as GLSL 1.0 does not support them. |
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.
// Note, this isn't a switch as GLSL 1.0 does not support them. | |
// Note, this isn't a switch as GLSL ES 1.0 does not support them. |
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.
Done
auto shader_source = std::string{ | ||
reinterpret_cast<const char*>(mapping.GetMapping()), mapping.GetSize()}; | ||
|
||
auto index = shader_source.find('\n'); |
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.
Could you add a comment that this is here to skip over the #version
line, which impellerc
is expected to always emit as the first line of a GLES shader? (Unless that's not what this is for)
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.
Done
const auto& constants = desc.GetSpecializationConstants(); | ||
|
||
std::vector<std::vector<vk::SpecializationMapEntry>> map_entries( | ||
desc.GetStageEntrypoints().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.
Do we need this outer vector? It looks like this could be a std::vector<vk::SpecializationMapEntry>
in the entrypoint loop.
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.
The VkSpecializationInfo holds onto a ptr to the VkSpecializationMapEntry, so we need to keep all of this alive/in scope until we actually create the pipeline.
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.
Ah gotcha.
*AVOID* adding specialization constants for color values or anything more complex. | ||
|
||
Specialization constants are provided to the CreateDefault argument in content_context.cc and aren't a | ||
part of variants. This is intentional: specialization constants shouldn't be used to create (potentially unlimited) runtime variants of a shader. |
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.
This along with restriction to 32bit ints both seem reasonable to me... Best not to hide our shader volume lest we accidentally land combinatorial explosions.
This PR and #47397 both have the chance of interacting with the advanced blend benchmark. In the interest of sanity I'm going to land that one first and sit on this one for another day or so. |
…137785) flutter/engine@2ec0f74...b11e318 2023-11-02 skia-flutter-autoroll@skia.org Roll Skia from 70634da5c783 to 2b3472da9888 (5 revisions) (flutter/engine#47608) 2023-11-02 jonahwilliams@google.com [Impeller] add support for specialization constants. (flutter/engine#47432) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Reland of #47432 Also includes: * #47617 * #47637 Fixes the performance on iOS by removing blocking on compilation of shaders. From local testing this has identical before/after numbers. Additional, ensures that we don't unecessarily specialize vertex shaders and notes this restriction in the documentation. ---- Adds support for Specialization constants to Impeller for our usage in the engine. A motivating example has been added in the impeller markdown docs. Fixes flutter/flutter#136210 Fixes flutter/flutter#119357
…47762) Reverts #47678 Initiated by: jonahwilliams This change reverts the following previous change: Original Description: Reland of #47432 Also includes: * #47617 * #47637 Fixes the performance on iOS by removing blocking on compilation of shaders. From local testing this has identical before/after numbers. Additional, ensures that we don't unecessarily specialize vertex shaders and notes this restriction in the documentation. ---- Adds support for Specialization constants to Impeller for our usage in the engine. A motivating example has been added in the impeller markdown docs. Fixes flutter/flutter#136210 Fixes flutter/flutter#119357
Reland of #47432 Also includes: #47617 #47637 Fixes the performance on iOS by removing blocking on compilation of shaders. From local testing this has identical before/after numbers. Additional, ensures that we don't unecessarily specialize vertex shaders and notes this restriction in the documentation. Adds support for Specialization constants to Impeller for our usage in the engine. A motivating example has been added in the impeller markdown docs. Fixes flutter/flutter#136210 Fixes flutter/flutter#119357 Investigating: flutter/flutter#138028
Adds support for Specialization constants to Impeller for our usage in the engine. A motivating example has been added in the impeller markdown docs.
Fixes flutter/flutter#136210
Fixes flutter/flutter#119357