Don't compile shaders to SkSL unless --sksl arg is present#182519
Don't compile shaders to SkSL unless --sksl arg is present#182519auto-submit[bot] merged 5 commits intoflutter:masterfrom
Conversation
github-actions
bot
added
engine
| compiler = CreateSkSLCompiler(ir, source_options); | ||
| break; | ||
| case TargetPlatform::kUnknown: | ||
| FML_UNREACHABLE(); |
There was a problem hiding this comment.
I thought this was mostly reserved for things that are truly unreachable? Maybe this is unreachable but one can't validate it by looking at this code where it appears. I think I'd rather have an FML_CHECK with an apropos error message here.
There was a problem hiding this comment.
This FML_UNREACHABLE is pretty much the same as the existing FML_UNREACHABLE for case TargetPlatform::kUnknown: in EntryPointMustBeNamedMain() above:
flutter/engine/src/flutter/impeller/compiler/compiler.cc
Lines 212 to 215 in e0e7a7d
I could add a FML_CHECK, but what would I return for this case? If this case ever happens, things are in a bad state and I think the old behavior of returning a GLSL compiler doesn't actually make sense.
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Impeller shader compiler (impellerc) to only compile shaders to SkSL when the --sksl argument is explicitly provided. Previously, SkSL compilation was triggered implicitly for any runtime shader target. The changes involve removing special-cased logic for SkSL and treating it as a regular compilation target. This required refactoring how compiler switches are handled, including making the target platform for CreateSourceOptions explicit and removing SelectDefaultTargetPlatform. The kKnownRuntimeStages was changed to a vector to preserve compilation order for tests. The pull request also includes significant updates to unit tests to improve coverage, adapt to the new behavior, and explicitly skip some newly-failing tests for Vulkan and SkSL to be addressed later. The changes are well-structured and improve the clarity and correctness of the compiler's command-line interface.
| if (GetParam() == TargetPlatform::kSkSL) { | ||
| GTEST_SKIP() << "Not supported with SkSL"; | ||
| } | ||
| if (GetParam() == TargetPlatform::kRuntimeStageVulkan) { | ||
| GTEST_SKIP() << "Not supported with Vulkan"; | ||
| } |
There was a problem hiding this comment.
As you noted in the PR description, skipping these tests for Vulkan and SkSL might be hiding a bug. It would be beneficial to add a // TODO comment with a link to a new or existing issue to track this. This will ensure these test gaps are addressed in the future. This also applies to the PositionedUniformsAppearInJson test below.
There was a problem hiding this comment.
Created an issue to follow up, and added comments here.
| SourceOptions options = | ||
| switches.CreateSourceOptions(switches.PlatformsToCompile().front()); |
There was a problem hiding this comment.
The choice to use .front() here is a bit subtle. To improve clarity for future maintainers, consider adding a comment explaining why this is safe. Based on your PR description, it seems this code path is for single-platform compilation, where PlatformsToCompile() will have only one element. A brief comment would capture this context.
There was a problem hiding this comment.
Added a comment about this.
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
Awesome, mostly looks good, just a few suggestions.
| namespace compiler { | ||
| namespace testing { | ||
|
|
||
| #define INSTANTIATE_TARGET_PLATFORM_TEST_SUITE_P(suite_name) \ |
There was a problem hiding this comment.
These macros are only used once, so why not just inline them?
There was a problem hiding this comment.
Done. Don't know why they were like this originally, but inlining them seems like a good change.
|
|
||
| TEST_P(CompilerTestRuntime, UniformsAppearInJson) { | ||
| if (GetParam() == TargetPlatform::kSkSL) { | ||
| GTEST_SKIP() << "Not supported with SkSL"; |
There was a problem hiding this comment.
For these skips, can we provide a couple more words about why they are not supported? Like, are the unimplemented today, but will be some day? Are they nonsensical, will they never be implemented, etc.
There was a problem hiding this comment.
After trying these out again, they do pass with SkSL but fail on Vulkan. Maybe some of the followup changes I made fixed SkSL somehow.
I filed #182578 to follow up for Vulkan. I don't know what the errors mean off the top of my head, but to me they look like they could possibly be something legit.
| // This TargetPlatform does not matter because it is overridden by | ||
| // GenerateShaderBackendFB(). | ||
| TargetPlatform::kUnknown)); |
There was a problem hiding this comment.
Maybe it makes sense to override the method then for no arguments instead of sending in data that just gets ignored.
There was a problem hiding this comment.
I changed CreateSourceOptions() to not take a TargetPlatform at all. The caller now needs to explicitly set the TargetPlatform after calling CreateSourceOptions(). I think this is a cleaner interface which accounts for some uses (specifically this one) not knowing the desired TargetPlatform until after CreateSourceOptions() is called.
- This used to incorrectly generate reflection state whenever the last shader_target_flags is not "--sksl".
- Instead, generate reflection state when any non-runtime target is in shader_target_flags.
- Consolidate some of the if/else logic to reduce duplicate code.
Remove the TargetPlatformNeedsReflection check in impeller_main.cc.
- Instead, whether reflection state is generated depends only on the presence or absense of "reflection_{json|header|cc}_name" flags.
- The logic of whether to include these flags is already in compiler.gni. So it's redundant to also have logic for whether to generate the reflection state here.
- The TargetPlatformNeedsReflection method had faulty logic.
- It returned true for everything except SkSL, even though reflection state isn't needed for runtime targets.
- It was called on the target from Switches::SelectDefaultTargetPlatform. When impellerc is used with multiple runtime targets, this would return the runtime target that is first alphabetically by flag name. So if --sksl is provided along with any other --runtime-stage-* target, SelectDefaultTargetPlatform returns the non-sksl runtime target. Effectively this meant that TargetPlatformNeedsReflection returns true except for when --sksl is the only provided runtime target.
- Removes the TargetPlatformNeedsReflection function entirely.
| // Creates source options from these switches. The returned options does not | ||
| // have a set TargetPlatform because that cannot be determined based purely | ||
| // on the switches. Clients must set a valid TargetPlatform on the returned | ||
| // options before before it is used. |
There was a problem hiding this comment.
This should be in the doxygen format (///)
|
The last commit came in after this was added to the merge queue. I'm not sure how that's going to shake out, not a huge deal in this case but just a heads up @b-luk |
|
Yeah, before the last submit, I added the autosubmit label and it was added to the merge queue. Then I realized I forgot the last commit, so I removed it from the merge queue to push the last commit. But the autosubmit re-added it to the queue before my commit actually got pushed. I would have thought that manually removing it from the merge queue would remove the autosubmit label as well, but I guess that's not the case. I would expect the merge queue will merge this with all the commits, not just the last one. Because as far as I can tell, the merge queue is based on PRs, not based on specific commits. If this is not the case, then we'll learn something new once this gets merged. I think it's worth the learning opportunity to see how it plays out. If it does turn out that it gets merged without the last commit, I can make a new PR for that commit. It's a very minor change so I don't think it would be a big deal if it does turn out it didn't get included here. |
flutter-dashboard
bot
removed
the
autosubmit
|
Unexpectedly (to me), it seems the last commit did not get included in the merge commit 3109939. The status of this PR at top of the page says "Merged auto-submit[bot] merged 5 commits into flutter:master", but only 4 commits were actually merged. I'll include the minor changes from the last commit as part of a subsequent PR. |
flutter/flutter@91b2d41...dad6f9d 2026-02-23 [email protected] Roll Packages from 9da22bf to 12b43a1 (19 revisions) (flutter/flutter#182758) 2026-02-23 [email protected] Roll Skia from 55fae1660572 to 9a5a3c92c336 (7 revisions) (flutter/flutter#182749) 2026-02-23 [email protected] Roll Skia from 256e2b47b303 to 55fae1660572 (1 revision) (flutter/flutter#182741) 2026-02-22 [email protected] Roll Skia from 141ed451f007 to 256e2b47b303 (1 revision) (flutter/flutter#182732) 2026-02-22 [email protected] Roll Skia from e59de8a12e7e to 141ed451f007 (1 revision) (flutter/flutter#182729) 2026-02-22 [email protected] Roll Skia from f6ea7ef42d14 to e59de8a12e7e (1 revision) (flutter/flutter#182726) 2026-02-22 [email protected] Roll Skia from 34fa7b2373f3 to f6ea7ef42d14 (1 revision) (flutter/flutter#182721) 2026-02-21 [email protected] Roll Skia from 3ca547c8b816 to 34fa7b2373f3 (2 revisions) (flutter/flutter#182711) 2026-02-21 [email protected] Roll Skia from c91ad88e89f8 to 3ca547c8b816 (9 revisions) (flutter/flutter#182696) 2026-02-21 [email protected] Shortcircuit if Tooltip message and richMessage are empty (flutter/flutter#182524) 2026-02-21 [email protected] Add fields getter to FormState (flutter/flutter#180815) 2026-02-20 [email protected] fix(web_ui): use static whitelist for image codec tests (flutter/flutter#182648) 2026-02-20 [email protected] Add cupertino docimports for CupertinoPageTransitionsBuilder and fix typos (flutter/flutter#182685) 2026-02-20 [email protected] Fix Chat invite link (flutter/flutter#182675) 2026-02-20 [email protected] Roll Skia from ce5854495a3a to c91ad88e89f8 (58 revisions) (flutter/flutter#182678) 2026-02-20 [email protected] Manual dart sdk flutter 174bcc79 25ff 4267 8e26 d0e902f18681 1771486449 (flutter/flutter#182624) 2026-02-20 [email protected] Don't compile shaders to SkSL unless --sksl arg is present (flutter/flutter#182519) 2026-02-20 [email protected] Correct PerformanceOverlay optionsMask checks and add tests (flutter/flutter#182309) 2026-02-20 [email protected] [Impeller] libImpeller: Dispose thread local caches on each Vulkan frame (flutter/flutter#182402) 2026-02-20 [email protected] Update CHANGELOG for 3.41.2 stable hotfix (flutter/flutter#182647) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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
…82519) The main change of this PR is to make `impellerc_main` no longer use `TargetPlatformBundlesSkSL(switches.SelectDefaultTargetPlatform())` to determine whether to compile to SkSL. Addresses the "sksl unexpectedly compiling when building for ios" mentioned in flutter#182400. `TargetPlatformBundlesSkSL()` returned `true` when running impellerc targeting *any* runtime shaders. So even though https://github.com/flutter/flutter/pull/163144/changes changed iOS runtime shaders to no longer call impellerc with the `--sksl` runtime target, it would still compile to SkSL because of the `--runtime-stage-metal` runtime target. After this PR, impellerc will only compile to SkSL if `--sksl` is explicitly provided when calling impellerc. The rest of the changes in this PR are related changes to improve code organization and to update tests. - `impellerc_main.cc`: - `OutputIPLR()`: Remove the custom logic for compiling to SkSL. SkSL is now treated the same as all the other compile targets in the `for (const auto& platform : switches.PlatformsToCompile())` loop. This is the only change with a behavioral difference in the PR. We no longer use `TargetPlatformBundlesSkSL(switches.SelectDefaultTargetPlatform())` to determine whether to compile to SkSL. Instead, it's treated the same as any other compilation target and will only be targeted when it is specified as an arg to the executable. - Remove the `CompileSkSL()` helper function. This had the exact same logic as what is used for all the other compilation targets in the `for (const auto& platform : switches.PlatformsToCompile())` loop. So it is not needed. - `OutputDepfile()`: Remove the switch/case. The `TargetPlatform::kUnknown` case is unreachable, so this used the other case 100% of the time. - `Main()`: `switches.CreateSourceOptions()` no longer has a default platform target parameter. It doesn't matter which target it's called with here. Arbitrarily call it with `switches.PlatformsToCompile().front()`. - `types.h/cc`: - Remove the `TargetPlatformBundlesSkSL()` function. The only place this was used was for `impellerc_main.cc`'s special case logic for SkSL, which was removed as described above. - `switches.h/cc`: - Remove the optional/default parameter for `CreateSourceOptions()`. This used to fall back to calling `SelectDefaultTargetPlatform()` to determine the default value. I found this to be very unclear behavior. The only place this was actually called with the default parameter is in one function in `shader_bundle.cc` where the selected target platform does not actually matter, so it doesn't even need this somewhat-convoluted logic to select a platform with `SelectDefaultTargetPlatform()`. I changed `CreateSourceOptions()` to require an explicit parameter, which I think makes the function's behavior clearer by removing the confusing default parameter. - Remove the `SelectDefaultTargetPlatform()` function. I think it was a little unclear/nonobvious what this was actually returning. It was only used in 3 places, all of which were kind of confusing/unneeded: 1. In this same file, to pick the default target platform in `switches.CreateSourceOptions()`. As described above, this seems entirely unnecessary. 1. In `impeller_main.cc`, used in `TargetPlatformBundlesSkSL(switches.SelectDefaultTargetPlatform())` to determine whether to compile to SkSL. As described above, we don't want this behavior. I think it was also kind of confusing. 1. In `impeller_main.cc`, used for the switch/case in `OutputDepfile()`. As described above, this switch/case is entirely unnecessary. - Change the `kKnownRuntimeStages` map to be a vector of pairs. This is iterated through to populate the returned `runtime_stages_` list of `Switches::PlatformsToCompile()`. Making it a vector makes the `runtime_stages_` list maintain the ordering of `kKnownRuntimeStages` (previously, iterating through the map would iterate in alphabetical order of the keys). - `impellerc_main.cc`'s `OutputIPLR()` now compiles to targets based on `Switches::PlatformsToCompile()`, without special case logic to always compile to "sksl" first. Changing this to a vector with "sksl" as the first value preserves the original behavior of compiling to "sksl" before any other targets. We do this because certain tests that perform a failed shader compilation check specifically for an SkSL-based error message (e.g. [this one](https://github.com/flutter/flutter/blob/64866862f623ceeb45fd8be4782e8db8b58910c0/packages/flutter_tools/test/integration.shard/shader_compiler_test.dart#L150-L170)). So for these tests, we need to try/fail with the SkSL compiler first, before trying/failing with other compilers which would produce a different error message. - `shader_bundle.cc` - As described above, change the usage of `switches.CreateSourceOptions()` to require an explicit target platform parameter. This particular usage doesn't matter, so use `TargetPlatform::kUnknown` and add an explanatory comment. - `compiler.cc` - In `CreateCompiler()`, Add an `FML_UNREACHABLE` for the `TargetPlatform::kUnknown` case, instead of falling back to using a vulkan compiler. It doesn't make sense to call `CreateCompiler()` with `TargetPlatform::kUnknown`. And currently, it can't happen: The only use of `CreateCompiler()` is in the `Compiler` constructor on [line 432](https://github.com/flutter/flutter/blob/24ce716cfddfef201027c1a5fa2299a8aeffb03e/engine/src/flutter/impeller/compiler/compiler.cc#L432), and there is a check preventing `TargetPlatform::kUnknown` earlier on [line 292](https://github.com/flutter/flutter/blob/24ce716cfddfef201027c1a5fa2299a8aeffb03e/engine/src/flutter/impeller/compiler/compiler.cc#L292). - `compiler_unittests.cc`, `compiler_test.h` - The `INSTANTIATE_{TARGET|RUNTIME_TARGET|SKSL_TARGET}_PLATFORM_TEST_SUITE_P` defines were oddly located in the file in the middle of the tests. Move them to the top of the file. - `INSTANTIATE_TARGET_PLATFORM_TEST_SUITE_P` - Remove the `kSkSL` target. These tests seem to be specifically for non-runtime targets, so SkSL doesn't belong here. All of these tests had a filter to skip for SkSL, so none of them actually ran for SkSL. These skips are now removed. - Add the `kVulkan` target, so all non-runtime platform targets are covered: opengles, openglesdesktop, metaldesktop, metalios, vulkan. - `INSTANTIATE_RUNTIME_TARGET_PLATFORM_TEST_SUITE_P` - This used to only test with `kRuntimeStageMetal`. For better coverage, I added all other runtime stages to this. It now tests on the metal, gles, gles3, vulkan, and sksl runtime targets. - Two tests, `UniformsAppearInJson` and `PositionedUniformsAppearInJson` fail when running with vulkan and with sksl. I added skips for these. I haven't dug deeper, but the failures seem unexpected to me. It's possible that this is revealing a bug with the vulkan and sksl compilers. - `INSTANTIATE_UNKNOWN_TARGET_PLATFORM_TEST_SUITE_P` - Added this new define for running tests with `TargetPlatform::kUnknown`. - Added a `MustFailDueToUnknownPlatform` test for this case. - `fixtures/BUILD.gn`, `runtime_stage_unittests.cc` - For the `impellerc("runtime_stages")` build target, add `--sksl` to the impellerc flags. This preserves the existing behavior of these targets being compiled for SkSL. They used to compile for SkSL because other runtime targets are specified. But now impellerc only compiles to SkSL when `--sksl` is explicitly specified. - Create a new `impellerc("runtime_stages_non_sksl")` target that runs impellerc without `--sksl`. Use this for a new `ContainsExpectedShaderTypesNoSksl` test in the unit test file. That test is the same as the existing `ContainsExpectedShaderTypes` test, but using the non-sksl output from `impellerc("runtime_stages_non_sksl")`. ### Update for commit 2 of the PR: The original PR had an issue that failed CI because a build rule expected an impellerc output to include C++ reflection data, but the reflection data was not output. I added a sizable commit to address this: - Fix compiler.gni logic around when to generate reflection state. - This used to incorrectly generate reflection state whenever the last shader_target_flags is not "--sksl". - Instead, generate reflection state when any non-runtime target is in shader_target_flags. - Consolidate some of the if/else logic to reduce duplicate code. - Remove the TargetPlatformNeedsReflection check in impeller_main.cc. - Instead, whether reflection state is generated depends only on the presence or absense of "reflection_{json|header|cc}_name" flags. - The logic of whether to include these flags is already in compiler.gni. So it's redundant to also have logic for whether to generate the reflection state here. - The TargetPlatformNeedsReflection method had faulty logic. - It returned true for everything except SkSL, even though reflection state isn't needed for runtime targets. - It was called on the target from Switches::SelectDefaultTargetPlatform. When impellerc is used with multiple runtime targets, this would return the runtime target that is first alphabetically by flag name. So if --sksl is provided along with any other --runtime-stage-* target, SelectDefaultTargetPlatform returns the non-sksl runtime target. Effectively this meant that TargetPlatformNeedsReflection returns true except for when --sksl is the only provided runtime target. - Removes the TargetPlatformNeedsReflection function entirely. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
2 participants
The main change of this PR is to make
impellerc_mainno longer useTargetPlatformBundlesSkSL(switches.SelectDefaultTargetPlatform())to determine whether to compile to SkSL.Addresses the "sksl unexpectedly compiling when building for ios" mentioned in #182400.
TargetPlatformBundlesSkSL()returnedtruewhen running impellerc targeting any runtime shaders. So even though https://github.com/flutter/flutter/pull/163144/changes changed iOS runtime shaders to no longer call impellerc with the--skslruntime target, it would still compile to SkSL because of the--runtime-stage-metalruntime target.After this PR, impellerc will only compile to SkSL if
--skslis explicitly provided when calling impellerc.The rest of the changes in this PR are related changes to improve code organization and to update tests.
impellerc_main.cc:OutputIPLR(): Remove the custom logic for compiling to SkSL. SkSL is now treated the same as all the other compile targets in thefor (const auto& platform : switches.PlatformsToCompile())loop. This is the only change with a behavioral difference in the PR. We no longer useTargetPlatformBundlesSkSL(switches.SelectDefaultTargetPlatform())to determine whether to compile to SkSL. Instead, it's treated the same as any other compilation target and will only be targeted when it is specified as an arg to the executable.CompileSkSL()helper function. This had the exact same logic as what is used for all the other compilation targets in thefor (const auto& platform : switches.PlatformsToCompile())loop. So it is not needed.OutputDepfile(): Remove the switch/case. TheTargetPlatform::kUnknowncase is unreachable, so this used the other case 100% of the time.Main():switches.CreateSourceOptions()no longer has a default platform target parameter. It doesn't matter which target it's called with here. Arbitrarily call it withswitches.PlatformsToCompile().front().types.h/cc:TargetPlatformBundlesSkSL()function. The only place this was used was forimpellerc_main.cc's special case logic for SkSL, which was removed as described above.switches.h/cc:CreateSourceOptions(). This used to fall back to callingSelectDefaultTargetPlatform()to determine the default value. I found this to be very unclear behavior. The only place this was actually called with the default parameter is in one function inshader_bundle.ccwhere the selected target platform does not actually matter, so it doesn't even need this somewhat-convoluted logic to select a platform withSelectDefaultTargetPlatform(). I changedCreateSourceOptions()to require an explicit parameter, which I think makes the function's behavior clearer by removing the confusing default parameter.SelectDefaultTargetPlatform()function. I think it was a little unclear/nonobvious what this was actually returning. It was only used in 3 places, all of which were kind of confusing/unneeded:switches.CreateSourceOptions(). As described above, this seems entirely unnecessary.impeller_main.cc, used inTargetPlatformBundlesSkSL(switches.SelectDefaultTargetPlatform())to determine whether to compile to SkSL. As described above, we don't want this behavior. I think it was also kind of confusing.impeller_main.cc, used for the switch/case inOutputDepfile(). As described above, this switch/case is entirely unnecessary.kKnownRuntimeStagesmap to be a vector of pairs. This is iterated through to populate the returnedruntime_stages_list ofSwitches::PlatformsToCompile(). Making it a vector makes theruntime_stages_list maintain the ordering ofkKnownRuntimeStages(previously, iterating through the map would iterate in alphabetical order of the keys).impellerc_main.cc'sOutputIPLR()now compiles to targets based onSwitches::PlatformsToCompile(), without special case logic to always compile to "sksl" first. Changing this to a vector with "sksl" as the first value preserves the original behavior of compiling to "sksl" before any other targets. We do this because certain tests that perform a failed shader compilation check specifically for an SkSL-based error message (e.g. this one). So for these tests, we need to try/fail with the SkSL compiler first, before trying/failing with other compilers which would produce a different error message.shader_bundle.ccswitches.CreateSourceOptions()to require an explicit target platform parameter. This particular usage doesn't matter, so useTargetPlatform::kUnknownand add an explanatory comment.compiler.ccCreateCompiler(), Add anFML_UNREACHABLEfor theTargetPlatform::kUnknowncase, instead of falling back to using a vulkan compiler. It doesn't make sense to callCreateCompiler()withTargetPlatform::kUnknown. And currently, it can't happen: The only use ofCreateCompiler()is in theCompilerconstructor on line 432, and there is a check preventingTargetPlatform::kUnknownearlier on line 292.compiler_unittests.cc,compiler_test.hINSTANTIATE_{TARGET|RUNTIME_TARGET|SKSL_TARGET}_PLATFORM_TEST_SUITE_Pdefines were oddly located in the file in the middle of the tests. Move them to the top of the file.INSTANTIATE_TARGET_PLATFORM_TEST_SUITE_PkSkSLtarget. These tests seem to be specifically for non-runtime targets, so SkSL doesn't belong here. All of these tests had a filter to skip for SkSL, so none of them actually ran for SkSL. These skips are now removed.kVulkantarget, so all non-runtime platform targets are covered: opengles, openglesdesktop, metaldesktop, metalios, vulkan.INSTANTIATE_RUNTIME_TARGET_PLATFORM_TEST_SUITE_PkRuntimeStageMetal. For better coverage, I added all other runtime stages to this. It now tests on the metal, gles, gles3, vulkan, and sksl runtime targets.UniformsAppearInJsonandPositionedUniformsAppearInJsonfail when running with vulkan and with sksl. I added skips for these. I haven't dug deeper, but the failures seem unexpected to me. It's possible that this is revealing a bug with the vulkan and sksl compilers.INSTANTIATE_UNKNOWN_TARGET_PLATFORM_TEST_SUITE_PTargetPlatform::kUnknown.MustFailDueToUnknownPlatformtest for this case.fixtures/BUILD.gn,runtime_stage_unittests.ccimpellerc("runtime_stages")build target, add--skslto the impellerc flags. This preserves the existing behavior of these targets being compiled for SkSL. They used to compile for SkSL because other runtime targets are specified. But now impellerc only compiles to SkSL when--skslis explicitly specified.impellerc("runtime_stages_non_sksl")target that runs impellerc without--sksl. Use this for a newContainsExpectedShaderTypesNoSksltest in the unit test file. That test is the same as the existingContainsExpectedShaderTypestest, but using the non-sksl output fromimpellerc("runtime_stages_non_sksl").Update for commit 2 of the PR:
The original PR had an issue that failed CI because a build rule expected an impellerc output to include C++ reflection data, but the reflection data was not output. I added a sizable commit to address this:
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.