-
Notifications
You must be signed in to change notification settings - Fork 6k
[Impeller] Use impellerc for engine FragmentProgram unit tests #33824
Conversation
chinmaygarde
left a comment
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.
Some nits but otherwise LGTM.
| case TargetPlatform::kFlutterSPIRV: | ||
| spirv_options.SetOptimizationLevel( | ||
| shaderc_optimization_level::shaderc_optimization_level_size); | ||
| shaderc_optimization_level::shaderc_optimization_level_zero); |
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.
Curious, why?
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 transpiler is missing some ops: flutter/flutter#105396. Added a comment.
Also it's the level being used by the current test harness:
engine/lib/spirv/test/glsl_to_spirv.cc
Line 39 in d0ec220
| options.SetOptimizationLevel(shaderc_optimization_level_zero); |
| ] | ||
| } | ||
|
|
||
| executable("glsl_to_spirv") { |
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.
Why can't this entire GN file be removed?
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 program above is still used. It looks like it is used for assembling invalid bytecode to make sure that the transpiler throws errors correctly.
570462a to
7fb0612
Compare
Since we're shipping
impellercto the framework and using it to compile shaders there, that's what we should be using to run the unit tests in the engine repo.