[Impeller] Remove redundant TargetPlatformNeedsSL. (flutter#40857)

TargetPlatformNeedsSL was true for all target platforms. I believe we reworked this after we started embedding metadata in IPLR files perhaps? In any case, this seemed unused and was making the code harder to read. Just a cleanup that I didn't want to include in the compiler rework for SPIRV.
Hixie · Apr 1, 2023 · c4c7e62 · c4c7e62
1 parent 0adfe35
commit c4c7e62
Show file tree

Hide file tree

Showing 6 changed files with 55 additions and 86 deletions.
diff --git a/impeller/compiler/compiler.cc b/impeller/compiler/compiler.cc
@@ -362,8 +362,7 @@ Compiler::Compiler(const fml::Mapping& source_mapping,
   shaderc::CompileOptions spirv_options;
 
   // Make sure reflection is as effective as possible. The generated shaders
-  // will be processed later by backend specific compilers. So optimizations
-  // here are irrelevant and get in the way of generating reflection code.
+  // will be processed later by backend specific compilers.
   spirv_options.SetGenerateDebugInfo();
 
   switch (options_.source_language) {
@@ -511,11 +510,6 @@ Compiler::Compiler(const fml::Mapping& source_mapping,
     included_file_names_ = std::move(included_file_names);
   }
 
-  if (!TargetPlatformNeedsSL(source_options.target_platform)) {
-    is_valid_ = true;
-    return;
-  }
-
   // SL Generation.
   spirv_cross::Parser parser(spv_result_->cbegin(),
                              spv_result_->cend() - spv_result_->cbegin());

diff --git a/impeller/compiler/compiler_test.cc b/impeller/compiler/compiler_test.cc
@@ -107,19 +107,17 @@ bool CompilerTest::CanCompileAndReflect(const char* fixture_name,
     return false;
   }
 
-  if (TargetPlatformNeedsSL(GetParam())) {
-    auto sl_source = compiler.GetSLShaderSource();
-    if (!sl_source) {
-      VALIDATION_LOG << "No SL source was generated.";
-      return false;
-    }
+  auto sl_source = compiler.GetSLShaderSource();
+  if (!sl_source) {
+    VALIDATION_LOG << "No SL source was generated.";
+    return false;
+  }
 
-    if (!fml::WriteAtomically(intermediates_directory_,
-                              SLFileName(fixture_name, GetParam()).c_str(),
-                              *sl_source)) {
-      VALIDATION_LOG << "Could not write SL intermediates.";
-      return false;
-    }
+  if (!fml::WriteAtomically(intermediates_directory_,
+                            SLFileName(fixture_name, GetParam()).c_str(),
+                            *sl_source)) {
+    VALIDATION_LOG << "Could not write SL intermediates.";
+    return false;
   }
 
   if (TargetPlatformNeedsReflection(GetParam())) {

diff --git a/impeller/compiler/impellerc_main.cc b/impeller/compiler/impellerc_main.cc
@@ -121,52 +121,49 @@ bool Main(const fml::CommandLine& command_line) {
     return false;
   }
 
-  if (TargetPlatformNeedsSL(options.target_platform)) {
-    auto sl_file_name = std::filesystem::absolute(
-        std::filesystem::current_path() / switches.sl_file_name);
-    const bool is_runtime_stage_data = switches.iplr;
-    if (is_runtime_stage_data) {
-      auto reflector = compiler.GetReflector();
-      if (reflector == nullptr) {
-        std::cerr << "Could not create reflector." << std::endl;
-        return false;
-      }
-      auto stage_data = reflector->GetRuntimeStageData();
-      if (!stage_data) {
-        std::cerr << "Runtime stage information was nil." << std::endl;
-        return false;
-      }
-      if (sksl_mapping) {
-        stage_data->SetSkSLData(sksl_mapping);
-      }
-      auto stage_data_mapping = options.json_format
-                                    ? stage_data->CreateJsonMapping()
-                                    : stage_data->CreateMapping();
-      if (!stage_data_mapping) {
-        std::cerr << "Runtime stage data could not be created." << std::endl;
-        return false;
-      }
-      if (!fml::WriteAtomically(*switches.working_directory,         //
-                                Utf8FromPath(sl_file_name).c_str(),  //
-                                *stage_data_mapping                  //
-                                )) {
-        std::cerr << "Could not write file to " << switches.sl_file_name
-                  << std::endl;
-        return false;
-      }
-      // Tools that consume the runtime stage data expect the access mode to
-      // be 0644.
-      if (!SetPermissiveAccess(sl_file_name)) {
-        return false;
-      }
-    } else {
-      if (!fml::WriteAtomically(*switches.working_directory,
-                                Utf8FromPath(sl_file_name).c_str(),
-                                *compiler.GetSLShaderSource())) {
-        std::cerr << "Could not write file to " << switches.sl_file_name
-                  << std::endl;
-        return false;
-      }
+  auto sl_file_name = std::filesystem::absolute(
+      std::filesystem::current_path() / switches.sl_file_name);
+  if (switches.iplr) {
+    auto reflector = compiler.GetReflector();
+    if (reflector == nullptr) {
+      std::cerr << "Could not create reflector." << std::endl;
+      return false;
+    }
+    auto stage_data = reflector->GetRuntimeStageData();
+    if (!stage_data) {
+      std::cerr << "Runtime stage information was nil." << std::endl;
+      return false;
+    }
+    if (sksl_mapping) {
+      stage_data->SetSkSLData(sksl_mapping);
+    }
+    auto stage_data_mapping = options.json_format
+                                  ? stage_data->CreateJsonMapping()
+                                  : stage_data->CreateMapping();
+    if (!stage_data_mapping) {
+      std::cerr << "Runtime stage data could not be created." << std::endl;
+      return false;
+    }
+    if (!fml::WriteAtomically(*switches.working_directory,         //
+                              Utf8FromPath(sl_file_name).c_str(),  //
+                              *stage_data_mapping                  //
+                              )) {
+      std::cerr << "Could not write file to " << switches.sl_file_name
+                << std::endl;
+      return false;
+    }
+    // Tools that consume the runtime stage data expect the access mode to
+    // be 0644.
+    if (!SetPermissiveAccess(sl_file_name)) {
+      return false;
+    }
+  } else {
+    if (!fml::WriteAtomically(*switches.working_directory,
+                              Utf8FromPath(sl_file_name).c_str(),
+                              *compiler.GetSLShaderSource())) {
+      std::cerr << "Could not write file to " << switches.sl_file_name
+                << std::endl;
+      return false;
     }
   }
 

diff --git a/impeller/compiler/switches.cc b/impeller/compiler/switches.cc
@@ -214,7 +214,7 @@ bool Switches::AreValid(std::ostream& explain) const {
     valid = false;
   }
 
-  if (sl_file_name.empty() && TargetPlatformNeedsSL(target_platform)) {
+  if (sl_file_name.empty()) {
     explain << "Target shading language file name was empty." << std::endl;
     valid = false;
   }

diff --git a/impeller/compiler/types.cc b/impeller/compiler/types.cc
@@ -123,24 +123,6 @@ std::string EntryPointFunctionNameFromSourceName(
   return stream.str();
 }
 
-bool TargetPlatformNeedsSL(TargetPlatform platform) {
-  switch (platform) {
-    case TargetPlatform::kMetalIOS:
-    case TargetPlatform::kMetalDesktop:
-    case TargetPlatform::kOpenGLES:
-    case TargetPlatform::kOpenGLDesktop:
-    case TargetPlatform::kRuntimeStageMetal:
-    case TargetPlatform::kRuntimeStageGLES:
-    case TargetPlatform::kRuntimeStageVulkan:
-    case TargetPlatform::kSkSL:
-    case TargetPlatform::kVulkan:
-      return true;
-    case TargetPlatform::kUnknown:
-      return false;
-  }
-  FML_UNREACHABLE();
-}
-
 bool TargetPlatformNeedsReflection(TargetPlatform platform) {
   switch (platform) {
     case TargetPlatform::kMetalIOS:

diff --git a/impeller/compiler/types.h b/impeller/compiler/types.h
@@ -66,8 +66,6 @@ std::string EntryPointFunctionNameFromSourceName(
     SourceLanguage source_language,
     const std::string& entry_point_name);
 
-bool TargetPlatformNeedsSL(TargetPlatform platform);
-
 bool TargetPlatformNeedsReflection(TargetPlatform platform);
 
 bool TargetPlatformBundlesSkSL(TargetPlatform platform);