[impellerc] Adds an SkSL backend #34441

zanderso · 2022-07-02T15:56:49Z

This PR adds a Compiler implementation to impellerc that translates SPIRV to SkSL. It is implemented by inheriting from the SPIRV to GLSL compiler, rejecting some features that aren't supported, and massaging the output slightly to look like an SkSL SkRuntimeEffect shader.

This PR also modifies the FragmentProgram.compile() API to accept a raw optional named argument, to which the SkSL can be passed. This is the same API that will be needed when we want to pass the backend-specific output when impeller is enabled.

Fixes flutter/flutter#106823

flutter-dashboard · 2022-07-02T15:56:51Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

flutter-dashboard · 2022-07-02T16:07:00Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

zanderso · 2022-07-08T22:25:09Z

I've done the best I can to exclude features that can't be translated to SkSL, but it's likely that I've missed some cases. The failure mode there is that the SkSL will fail to compile at runtime rather than at app build time.

Most of the heavy lifting is in spirv_sksl.cc. The FragmentProgram API changes are in painting.dart, and the rest is plumbing and tests.

jonahwilliams · 2022-07-08T22:32:54Z

lib/ui/painting.dart

@@ -3906,27 +3906,65 @@ class FragmentProgram extends NativeFieldWrapperClass1 {
  /// [A current specification of valid SPIR-V is here.](https://github.com/flutter/engine/blob/master/lib/spirv/README.md)
  /// SPIR-V not meeting this specification will throw an exception.
  static Future<FragmentProgram> compile({


I don't think we should combine this new functionality into the old API. Instead we should start from scratch with a new one and deprecate this.

maybe we'd always want to retain an API for loading bytes from the dart heap - just to support certain dynamic cases, but for bundled assets ideally we would only pass a reference to the asset (probably the string key) and let the engine do the loading.

That would also allow me to support hot reload with no framework side changes.

Also, can impellerc include uniform counts into the blob it creates so users don't need to manage that?

Yeah all of that makes sense to me. Not sure what new API here would make life easiest. Do you have any suggestions?

Something like:

FragmentProgram.fromAsset(String key)

or if needs to be async, then

static Future<FragmentProgram> fromAsset(String key)

Depending on the ownership needs of the engine, we could also use immutable buffer but that implies at least one copy.

Where the key would be an asset containing platform specific (metal, sksl, et cetera) blob that had enough information.

That said, we also don't need to come up with this API in this PR, and could instead leave the old API for now and land the compiler changes to allow for experimentation?

chinmaygarde

I reviewed the impellerc and testing bits and this is great start. I agree with @jonahwilliams that we don't need to modify the current API or come up with a new one in this PR. Separately, I think we can also update the impellerc unittests harness to compile the SKSL shaders generated to verify correctness. Some nits but otherwise LGTM.

chinmaygarde · 2022-07-09T20:42:59Z

impeller/compiler/compiler.cc

@@ -151,6 +160,7 @@ Compiler::Compiler(const fml::Mapping& source_mapping,
          shaderc_spirv_version::shaderc_spirv_version_1_0);
      break;
    case TargetPlatform::kFlutterSPIRV:
+    case TargetPlatform::kSkSL:  // TODO(zra): Allow optimizations.


Let's file an issue for this and cross reference it here? Curious, did you try to see what happens? We can figure it out later though.

The SkSL loop analysis code doesn't correctly handle everything that's allowed by the grammar. Added a comment, and filed an issue.

chinmaygarde · 2022-07-09T20:44:30Z

impeller/compiler/compiler_backend.h


 namespace impeller {
 namespace compiler {

 struct CompilerBackend {
  using MSLCompiler = std::shared_ptr<spirv_cross::CompilerMSL>;
  using GLSLCompiler = std::shared_ptr<spirv_cross::CompilerGLSL>;
-  using Compiler = std::variant<MSLCompiler, GLSLCompiler>;
+  using SkSLCompiler = std::shared_ptr<CompilerSkSL>;
+  using Compiler = std::variant<MSLCompiler, GLSLCompiler, SkSLCompiler>;


Ooh, sorry about this. This is a cleanup that is tracked in flutter/flutter#106376.

zanderso · 2022-07-11T15:52:17Z

I think what I should do is:

Wait for the beta branch hash to be selected this week.
Land this.
Make sure it works with ink_sparkle.
Add a new API, and then migrate to that. Sketch is here: zanderso@a1ee546.

brianosman · 2022-07-12T20:32:31Z

impeller/compiler/spirv_sksl.cc

+  statement("// This SkSL shader is autogenerated by spirv-cross.");
+  statement("");
+
+  // This builtin is copied from


This is unlikely to keep working (it's an accident that it works today). I'm curious about the semantics you expect to get here -- are the shaders you're using being written to use gl_FragCoord? We've deliberately hidden that from people in SkSL for several reasons:

gl_FragCoord is origin-relative (ie, often bottom-left). sk_FragCoord is always top-left relative.

These coordinates are always in device space, so they aren't impact by canvas transformations (this is generally not what people want).
2b) More importantly, because they're in device space, they expose people to the implementation details of things like saveLayer.

The near-term goal is to mimic the SPIR-V -> SkSL transpiler in the Engine so that we can remove it. The main constraint is that we can't break the ink_sparkle shader. The ink_sparkle shader references gl_FragCoord, and the existing transpiler translates that to float4(iFragCoord, 0, 0), where iFragCoord is the input of SkSL's main(). I've modified this PR to do the same thing instead of hacking in sk_FragCoord.

It sounds like going forward we'll have to think a bit harder about this setup if we're going to aim for parity between the different backends.

zanderso added the Work in progress (WIP) Not ready (yet) for review! label Jul 2, 2022

flutter-dashboard bot added the needs tests label Jul 2, 2022

zanderso marked this pull request as draft July 2, 2022 16:02

zanderso force-pushed the impellerc-sksl branch 6 times, most recently from 4618e3e to 754ea63 Compare July 8, 2022 21:27

zanderso removed Work in progress (WIP) Not ready (yet) for review! needs tests labels Jul 8, 2022

zanderso marked this pull request as ready for review July 8, 2022 22:07

zanderso requested review from chinmaygarde, dnfield and jonahwilliams July 8, 2022 22:10

jonahwilliams reviewed Jul 8, 2022

View reviewed changes

chinmaygarde added the e: impeller label Jul 9, 2022

flutter-dashboard bot added the platform-web Code specifically for the web engine label Jul 9, 2022

chinmaygarde approved these changes Jul 9, 2022

View reviewed changes

chinmaygarde assigned zanderso Jul 9, 2022

zanderso force-pushed the impellerc-sksl branch 2 times, most recently from b08f01b to 0827891 Compare July 11, 2022 15:33

brianosman reviewed Jul 12, 2022

View reviewed changes

zanderso added 3 commits July 13, 2022 09:03

[impellerc] Adds an SkSL backend

504e9b3

WIP

e5176b5

WIP

ed467d5

zanderso added 5 commits July 13, 2022 09:03

WIP

fa9c64b

WIP

4565abd

WIP

5c0c769

Address comments

ad1d165

Remove reference to sk_FragCoord

114bfb5

zanderso force-pushed the impellerc-sksl branch from 0827891 to 114bfb5 Compare July 13, 2022 16:05

zanderso merged commit 0b36476 into flutter:main Jul 13, 2022

zanderso deleted the impellerc-sksl branch July 13, 2022 22:05

zanderso restored the impellerc-sksl branch July 13, 2022 22:05

engine-flutter-autoroll mentioned this pull request Jul 13, 2022

Roll Flutter Engine from 06fa87a934a2 to 495ed0367841 (4 revisions) flutter/flutter#107594

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 13, 2022

0b36476 [impellerc] Adds an SkSL backend (flutter/engine#34441)

aaad429

betrevisan pushed a commit to betrevisan/engine that referenced this pull request Jul 15, 2022

[impellerc] Adds an SkSL backend (flutter#34441)

50d1c79

zanderso deleted the impellerc-sksl branch July 19, 2022 02:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[impellerc] Adds an SkSL backend #34441

[impellerc] Adds an SkSL backend #34441

zanderso commented Jul 2, 2022 •

edited by chinmaygarde

Loading

flutter-dashboard bot commented Jul 2, 2022

flutter-dashboard bot commented Jul 2, 2022

zanderso commented Jul 8, 2022

jonahwilliams Jul 8, 2022

zanderso Jul 8, 2022

jonahwilliams Jul 8, 2022

chinmaygarde left a comment

chinmaygarde Jul 9, 2022

zanderso Jul 11, 2022

chinmaygarde Jul 9, 2022

zanderso commented Jul 11, 2022

brianosman Jul 12, 2022

zanderso Jul 13, 2022

[impellerc] Adds an SkSL backend #34441

[impellerc] Adds an SkSL backend #34441

Conversation

zanderso commented Jul 2, 2022 • edited by chinmaygarde Loading

flutter-dashboard bot commented Jul 2, 2022

flutter-dashboard bot commented Jul 2, 2022

zanderso commented Jul 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinmaygarde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanderso commented Jul 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanderso commented Jul 2, 2022 •

edited by chinmaygarde

Loading