From d8d6d161d7a853f85129e4fdcfbf384b0225cc6f Mon Sep 17 00:00:00 2001 From: Benson Luk <97480502+b-luk@users.noreply.github.com> Date: Wed, 18 Feb 2026 15:37:01 -0800 Subject: [PATCH] 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. --- .../impeller/compiler/compiler_test.cc | 77 +++++++++---------- .../impeller/compiler/impellerc_main.cc | 63 ++++++++------- engine/src/flutter/impeller/compiler/types.cc | 19 ----- engine/src/flutter/impeller/compiler/types.h | 2 - .../src/flutter/impeller/tools/compiler.gni | 70 ++++++++--------- 5 files changed, 97 insertions(+), 134 deletions(-) diff --git a/engine/src/flutter/impeller/compiler/compiler_test.cc b/engine/src/flutter/impeller/compiler/compiler_test.cc index 683e6bea578..5dc1d566ad4 100644 --- a/engine/src/flutter/impeller/compiler/compiler_test.cc +++ b/engine/src/flutter/impeller/compiler/compiler_test.cc @@ -142,53 +142,50 @@ bool CompilerTestBase::CanCompileAndReflect( return false; } - if (TargetPlatformNeedsReflection(GetParam())) { - auto reflector = compiler.GetReflector(); - if (!reflector) { - VALIDATION_LOG - << "No reflector was found for target platform SL compiler."; - return false; - } + auto reflector = compiler.GetReflector(); + if (!reflector) { + VALIDATION_LOG << "No reflector was found for target platform SL compiler."; + return false; + } - auto reflection_json = reflector->GetReflectionJSON(); - auto reflection_header = reflector->GetReflectionHeader(); - auto reflection_source = reflector->GetReflectionCC(); + auto reflection_json = reflector->GetReflectionJSON(); + auto reflection_header = reflector->GetReflectionHeader(); + auto reflection_source = reflector->GetReflectionCC(); - if (!reflection_json) { - VALIDATION_LOG << "Reflection JSON was not found."; - return false; - } + if (!reflection_json) { + VALIDATION_LOG << "Reflection JSON was not found."; + return false; + } - if (!reflection_header) { - VALIDATION_LOG << "Reflection header was not found."; - return false; - } + if (!reflection_header) { + VALIDATION_LOG << "Reflection header was not found."; + return false; + } - if (!reflection_source) { - VALIDATION_LOG << "Reflection source was not found."; - return false; - } + if (!reflection_source) { + VALIDATION_LOG << "Reflection source was not found."; + return false; + } - if (!fml::WriteAtomically(intermediates_directory_, - ReflectionHeaderName(fixture_name).c_str(), - *reflection_header)) { - VALIDATION_LOG << "Could not write reflection header intermediates."; - return false; - } + if (!fml::WriteAtomically(intermediates_directory_, + ReflectionHeaderName(fixture_name).c_str(), + *reflection_header)) { + VALIDATION_LOG << "Could not write reflection header intermediates."; + return false; + } - if (!fml::WriteAtomically(intermediates_directory_, - ReflectionCCName(fixture_name).c_str(), - *reflection_source)) { - VALIDATION_LOG << "Could not write reflection CC intermediates."; - return false; - } + if (!fml::WriteAtomically(intermediates_directory_, + ReflectionCCName(fixture_name).c_str(), + *reflection_source)) { + VALIDATION_LOG << "Could not write reflection CC intermediates."; + return false; + } - if (!fml::WriteAtomically(intermediates_directory_, - ReflectionJSONName(fixture_name).c_str(), - *reflection_json)) { - VALIDATION_LOG << "Could not write reflection json intermediates."; - return false; - } + if (!fml::WriteAtomically(intermediates_directory_, + ReflectionJSONName(fixture_name).c_str(), + *reflection_json)) { + VALIDATION_LOG << "Could not write reflection json intermediates."; + return false; } return true; } diff --git a/engine/src/flutter/impeller/compiler/impellerc_main.cc b/engine/src/flutter/impeller/compiler/impellerc_main.cc index 37fdf29c030..13eb1a37296 100644 --- a/engine/src/flutter/impeller/compiler/impellerc_main.cc +++ b/engine/src/flutter/impeller/compiler/impellerc_main.cc @@ -116,43 +116,40 @@ static bool OutputReflectionData(const Compiler& compiler, /// May include a JSON file, a C++ header, and/or a C++ TU. /// - if (TargetPlatformNeedsReflection(options.target_platform)) { - if (!switches.reflection_json_name.empty()) { - auto reflection_json_name = std::filesystem::absolute( - std::filesystem::current_path() / switches.reflection_json_name); - if (!fml::WriteAtomically( - *switches.working_directory, - Utf8FromPath(reflection_json_name).c_str(), - *compiler.GetReflector()->GetReflectionJSON())) { - std::cerr << "Could not write reflection json to " - << switches.reflection_json_name << std::endl; - return false; - } + if (!switches.reflection_json_name.empty()) { + auto reflection_json_name = std::filesystem::absolute( + std::filesystem::current_path() / switches.reflection_json_name); + if (!fml::WriteAtomically(*switches.working_directory, + Utf8FromPath(reflection_json_name).c_str(), + *compiler.GetReflector()->GetReflectionJSON())) { + std::cerr << "Could not write reflection json to " + << switches.reflection_json_name << std::endl; + return false; } + } - if (!switches.reflection_header_name.empty()) { - auto reflection_header_name = std::filesystem::absolute( - std::filesystem::current_path() / switches.reflection_header_name); - if (!fml::WriteAtomically( - *switches.working_directory, - Utf8FromPath(reflection_header_name).c_str(), - *compiler.GetReflector()->GetReflectionHeader())) { - std::cerr << "Could not write reflection header to " - << switches.reflection_header_name << std::endl; - return false; - } + if (!switches.reflection_header_name.empty()) { + auto reflection_header_name = std::filesystem::absolute( + std::filesystem::current_path() / switches.reflection_header_name); + if (!fml::WriteAtomically( + *switches.working_directory, + Utf8FromPath(reflection_header_name).c_str(), + *compiler.GetReflector()->GetReflectionHeader())) { + std::cerr << "Could not write reflection header to " + << switches.reflection_header_name << std::endl; + return false; } + } - if (!switches.reflection_cc_name.empty()) { - auto reflection_cc_name = std::filesystem::absolute( - std::filesystem::current_path() / switches.reflection_cc_name); - if (!fml::WriteAtomically(*switches.working_directory, - Utf8FromPath(reflection_cc_name).c_str(), - *compiler.GetReflector()->GetReflectionCC())) { - std::cerr << "Could not write reflection CC to " - << switches.reflection_cc_name << std::endl; - return false; - } + if (!switches.reflection_cc_name.empty()) { + auto reflection_cc_name = std::filesystem::absolute( + std::filesystem::current_path() / switches.reflection_cc_name); + if (!fml::WriteAtomically(*switches.working_directory, + Utf8FromPath(reflection_cc_name).c_str(), + *compiler.GetReflector()->GetReflectionCC())) { + std::cerr << "Could not write reflection CC to " + << switches.reflection_cc_name << std::endl; + return false; } } return true; diff --git a/engine/src/flutter/impeller/compiler/types.cc b/engine/src/flutter/impeller/compiler/types.cc index 761c7a6b2cd..57723c372a4 100644 --- a/engine/src/flutter/impeller/compiler/types.cc +++ b/engine/src/flutter/impeller/compiler/types.cc @@ -127,25 +127,6 @@ std::string EntryPointFunctionNameFromSourceName( return stream.str(); } -bool TargetPlatformNeedsReflection(TargetPlatform platform) { - switch (platform) { - case TargetPlatform::kMetalIOS: - case TargetPlatform::kMetalDesktop: - case TargetPlatform::kOpenGLES: - case TargetPlatform::kOpenGLDesktop: - case TargetPlatform::kRuntimeStageMetal: - case TargetPlatform::kRuntimeStageGLES: - case TargetPlatform::kRuntimeStageGLES3: - case TargetPlatform::kRuntimeStageVulkan: - case TargetPlatform::kVulkan: - return true; - case TargetPlatform::kUnknown: - case TargetPlatform::kSkSL: - return false; - } - FML_UNREACHABLE(); -} - std::string ShaderCErrorToString(shaderc_compilation_status status) { using Status = shaderc_compilation_status; switch (status) { diff --git a/engine/src/flutter/impeller/compiler/types.h b/engine/src/flutter/impeller/compiler/types.h index 37f2fdc9db1..7adddff9e26 100644 --- a/engine/src/flutter/impeller/compiler/types.h +++ b/engine/src/flutter/impeller/compiler/types.h @@ -122,8 +122,6 @@ std::string EntryPointFunctionNameFromSourceName( SourceLanguage source_language, const std::string& entry_point_name); -bool TargetPlatformNeedsReflection(TargetPlatform platform); - std::string ShaderCErrorToString(shaderc_compilation_status status); shaderc_shader_kind ToShaderCShaderKind(SourceType type); diff --git a/engine/src/flutter/impeller/tools/compiler.gni b/engine/src/flutter/impeller/tools/compiler.gni index edaa4f9b65a..7435a7308ca 100644 --- a/engine/src/flutter/impeller/tools/compiler.gni +++ b/engine/src/flutter/impeller/tools/compiler.gni @@ -75,10 +75,6 @@ template("impellerc") { shader_target_flags = [] } - sksl = false - foreach(shader_target_flag, shader_target_flags) { - sksl = shader_target_flag == "--sksl" - } iplr = false if (defined(invoker.iplr) && invoker.iplr) { iplr = invoker.iplr @@ -91,7 +87,6 @@ template("impellerc") { # Not needed on every path. not_needed([ "iplr", - "sksl", "shader_bundle", "shader_bundle_output", ]) @@ -181,11 +176,7 @@ template("impellerc") { ] outputs = [ sl_output ] - } else if (sksl) { - # When SkSL is selected as a `shader_target_flags`, don't generate - # C++ reflection state. Nothing needs to use it and it's likely invalid - # given the special cases when generating SkSL. - # Note that this configuration is orthogonal to the "--iplr" flag + } else { sl_output = "$generated_dir/{{source_file_part}}.${invoker.sl_file_extension}" sl_output_path = rebase_path(sl_output, root_build_dir) @@ -198,40 +189,39 @@ template("impellerc") { ] outputs = [ sl_output ] - } else { - # The default branch. Here we just generate one shader along with all of - # its C++ reflection state. - sl_output = - "$generated_dir/{{source_file_part}}.${invoker.sl_file_extension}" - sl_output_path = rebase_path(sl_output, root_build_dir) + # Generate reflection state only if a non-runtime target is provided. + if (filter_include(shader_target_flags, + [ + "--metal-ios", + "--metal-desktop", + "--opengl-es", + "--opengl-desktop", + "--vulkan", + ]) != []) { + reflection_json_intermediate = + "$generated_dir/{{source_file_part}}.json" + reflection_header_intermediate = "$generated_dir/{{source_file_part}}.h" + reflection_cc_intermediate = "$generated_dir/{{source_file_part}}.cc" - reflection_json_intermediate = "$generated_dir/{{source_file_part}}.json" - reflection_header_intermediate = "$generated_dir/{{source_file_part}}.h" - reflection_cc_intermediate = "$generated_dir/{{source_file_part}}.cc" + reflection_json_path = + rebase_path(reflection_json_intermediate, root_build_dir) + reflection_header_path = + rebase_path(reflection_header_intermediate, root_build_dir) + reflection_cc_path = + rebase_path(reflection_cc_intermediate, root_build_dir) - spirv_intermediate = "$generated_dir/{{source_file_part}}.spirv" - spirv_intermediate_path = rebase_path(spirv_intermediate, root_build_dir) - reflection_json_path = - rebase_path(reflection_json_intermediate, root_build_dir) - reflection_header_path = - rebase_path(reflection_header_intermediate, root_build_dir) - reflection_cc_path = - rebase_path(reflection_cc_intermediate, root_build_dir) + args += [ + "--reflection-json=$reflection_json_path", + "--reflection-header=$reflection_header_path", + "--reflection-cc=$reflection_cc_path", + ] - args += [ - "--sl=$sl_output_path", - "--spirv=$spirv_intermediate_path", - "--reflection-json=$reflection_json_path", - "--reflection-header=$reflection_header_path", - "--reflection-cc=$reflection_cc_path", - ] - - outputs = [ - sl_output, - reflection_header_intermediate, - reflection_cc_intermediate, - ] + outputs += [ + reflection_header_intermediate, + reflection_cc_intermediate, + ] + } } if (defined(invoker.defines)) {