From 00376de3268cb447d50ca363c102cd365774cd9b Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Thu, 2 Feb 2023 22:25:03 -0800 Subject: [PATCH] Revert "[impellerc] sort uniforms on metal backend (#39345)" (flutter/engine#39356) This reverts commit abc97cd6e06320eda21e4f1ac277bf57ed13aa00. --- .../ci/licenses_golden/licenses_flutter | 4 -- engine/src/flutter/impeller/compiler/BUILD.gn | 2 - .../src/flutter/impeller/compiler/compiler.cc | 46 ------------------- .../impeller/compiler/compiler_backend.h | 4 +- .../flutter/impeller/compiler/reflector.cc | 37 +++++++-------- .../flutter/impeller/compiler/spirv_sksl.cc | 45 +++++++++++++++--- .../impeller/compiler/uniform_sorter.cc | 45 ------------------ .../impeller/compiler/uniform_sorter.h | 22 --------- .../contents/runtime_effect_contents.cc | 2 +- 9 files changed, 59 insertions(+), 148 deletions(-) delete mode 100644 engine/src/flutter/impeller/compiler/uniform_sorter.cc delete mode 100644 engine/src/flutter/impeller/compiler/uniform_sorter.h diff --git a/engine/src/flutter/ci/licenses_golden/licenses_flutter b/engine/src/flutter/ci/licenses_golden/licenses_flutter index e1361dda517..4d518c29f3b 100644 --- a/engine/src/flutter/ci/licenses_golden/licenses_flutter +++ b/engine/src/flutter/ci/licenses_golden/licenses_flutter @@ -1226,8 +1226,6 @@ ORIGIN: ../../../flutter/impeller/compiler/switches.cc + ../../../flutter/LICENS ORIGIN: ../../../flutter/impeller/compiler/switches.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/compiler/types.cc + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/compiler/types.h + ../../../flutter/LICENSE -ORIGIN: ../../../flutter/impeller/compiler/uniform_sorter.cc + ../../../flutter/LICENSE -ORIGIN: ../../../flutter/impeller/compiler/uniform_sorter.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/compiler/utilities.cc + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/compiler/utilities.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/display_list/display_list_dispatcher.cc + ../../../flutter/LICENSE @@ -3707,8 +3705,6 @@ FILE: ../../../flutter/impeller/compiler/switches.cc FILE: ../../../flutter/impeller/compiler/switches.h FILE: ../../../flutter/impeller/compiler/types.cc FILE: ../../../flutter/impeller/compiler/types.h -FILE: ../../../flutter/impeller/compiler/uniform_sorter.cc -FILE: ../../../flutter/impeller/compiler/uniform_sorter.h FILE: ../../../flutter/impeller/compiler/utilities.cc FILE: ../../../flutter/impeller/compiler/utilities.h FILE: ../../../flutter/impeller/display_list/display_list_dispatcher.cc diff --git a/engine/src/flutter/impeller/compiler/BUILD.gn b/engine/src/flutter/impeller/compiler/BUILD.gn index 49a5ec2ab66..dc06f32fd42 100644 --- a/engine/src/flutter/impeller/compiler/BUILD.gn +++ b/engine/src/flutter/impeller/compiler/BUILD.gn @@ -52,8 +52,6 @@ impeller_component("compiler_lib") { "switches.h", "types.cc", "types.h", - "uniform_sorter.cc", - "uniform_sorter.h", ] public_deps = [ diff --git a/engine/src/flutter/impeller/compiler/compiler.cc b/engine/src/flutter/impeller/compiler/compiler.cc index a3a67c8f8d5..edb9fe69354 100644 --- a/engine/src/flutter/impeller/compiler/compiler.cc +++ b/engine/src/flutter/impeller/compiler/compiler.cc @@ -16,7 +16,6 @@ #include "impeller/compiler/includer.h" #include "impeller/compiler/logger.h" #include "impeller/compiler/types.h" -#include "impeller/compiler/uniform_sorter.h" namespace impeller { namespace compiler { @@ -37,51 +36,6 @@ static CompilerBackend CreateMSLCompiler(const spirv_cross::ParsedIR& ir, spirv_cross::CompilerMSL::Options::make_msl_version(1, 2); sl_compiler->set_msl_options(sl_options); - // Sort the float and sampler uniforms according to their declared/decorated - // order. For user authored fragment shaders, the API for setting uniform - // values uses the index of the uniform in the declared order. By default, the - // metal backend of spirv-cross will order uniforms according to usage. To fix - // this, we use the sorted order and the add_msl_resource_binding API to force - // the ordering to match the declared order. Note that while this code runs - // for all compiled shaders, it will only affect fragment shaders due to the - // specified stage. - auto floats = - SortUniforms(&ir, sl_compiler.get(), spirv_cross::SPIRType::Float); - auto images = - SortUniforms(&ir, sl_compiler.get(), spirv_cross::SPIRType::SampledImage); - - uint32_t buffer_offset = 0; - uint32_t sampler_offset = 0; - for (auto& float_id : floats) { - sl_compiler->add_msl_resource_binding( - {.stage = spv::ExecutionModel::ExecutionModelFragment, - .basetype = spirv_cross::SPIRType::BaseType::Float, - .desc_set = sl_compiler->get_decoration(float_id, - spv::DecorationDescriptorSet), - .binding = - sl_compiler->get_decoration(float_id, spv::DecorationBinding), - .count = 1u, - .msl_buffer = buffer_offset}); - buffer_offset++; - } - for (auto& image_id : images) { - sl_compiler->add_msl_resource_binding({ - .stage = spv::ExecutionModel::ExecutionModelFragment, - .basetype = spirv_cross::SPIRType::BaseType::SampledImage, - .desc_set = - sl_compiler->get_decoration(image_id, spv::DecorationDescriptorSet), - .binding = - sl_compiler->get_decoration(image_id, spv::DecorationBinding), - .count = 1u, - // A sampled image is both an image and a sampler, so both - // offsets need to be set or depending on the partiular shader - // the bindings may be incorrect. - .msl_texture = sampler_offset, - .msl_sampler = sampler_offset, - }); - sampler_offset++; - } - return CompilerBackend(sl_compiler); } diff --git a/engine/src/flutter/impeller/compiler/compiler_backend.h b/engine/src/flutter/impeller/compiler/compiler_backend.h index deb57e92351..b8225fce186 100644 --- a/engine/src/flutter/impeller/compiler/compiler_backend.h +++ b/engine/src/flutter/impeller/compiler/compiler_backend.h @@ -44,8 +44,6 @@ struct CompilerBackend { const spirv_cross::Compiler* operator->() const; - spirv_cross::Compiler* GetCompiler(); - operator bool() const; enum class ExtendedResourceIndex { @@ -57,6 +55,8 @@ struct CompilerBackend { const spirv_cross::Compiler* GetCompiler() const; + spirv_cross::Compiler* GetCompiler(); + private: Type type_ = Type::kMSL; Compiler compiler_; diff --git a/engine/src/flutter/impeller/compiler/reflector.cc b/engine/src/flutter/impeller/compiler/reflector.cc index 09ad507111f..9b911dfef4d 100644 --- a/engine/src/flutter/impeller/compiler/reflector.cc +++ b/engine/src/flutter/impeller/compiler/reflector.cc @@ -16,7 +16,6 @@ #include "impeller/base/strings.h" #include "impeller/base/validation.h" #include "impeller/compiler/code_gen_template.h" -#include "impeller/compiler/uniform_sorter.h" #include "impeller/compiler/utilities.h" #include "impeller/geometry/matrix.h" #include "impeller/geometry/scalar.h" @@ -360,25 +359,23 @@ std::shared_ptr Reflector::GenerateRuntimeStageData() const { if (sksl_data_) { data->SetSkSLData(sksl_data_); } - - // Sort the IR so that the uniforms are in declaration order. - std::vector uniforms = - SortUniforms(ir_.get(), compiler_.GetCompiler()); - - for (auto& sorted_id : uniforms) { - auto var = ir_->ids[sorted_id].get(); - const auto spir_type = compiler_->get_type(var.basetype); - UniformDescription uniform_description; - uniform_description.name = compiler_->get_name(var.self); - uniform_description.location = compiler_->get_decoration( - var.self, spv::Decoration::DecorationLocation); - uniform_description.type = spir_type.basetype; - uniform_description.rows = spir_type.vecsize; - uniform_description.columns = spir_type.columns; - uniform_description.bit_width = spir_type.width; - uniform_description.array_elements = GetArrayElements(spir_type); - data->AddUniformDescription(std::move(uniform_description)); - } + ir_->for_each_typed_id( + [&](uint32_t, const spirv_cross::SPIRVariable& var) { + if (var.storage != spv::StorageClassUniformConstant) { + return; + } + const auto spir_type = compiler_->get_type(var.basetype); + UniformDescription uniform_description; + uniform_description.name = compiler_->get_name(var.self); + uniform_description.location = compiler_->get_decoration( + var.self, spv::Decoration::DecorationLocation); + uniform_description.type = spir_type.basetype; + uniform_description.rows = spir_type.vecsize; + uniform_description.columns = spir_type.columns; + uniform_description.bit_width = spir_type.width; + uniform_description.array_elements = GetArrayElements(spir_type); + data->AddUniformDescription(std::move(uniform_description)); + }); return data; } diff --git a/engine/src/flutter/impeller/compiler/spirv_sksl.cc b/engine/src/flutter/impeller/compiler/spirv_sksl.cc index c3b0f418b28..287abf54014 100644 --- a/engine/src/flutter/impeller/compiler/spirv_sksl.cc +++ b/engine/src/flutter/impeller/compiler/spirv_sksl.cc @@ -3,7 +3,6 @@ // found in the LICENSE file. #include "impeller/compiler/spirv_sksl.h" -#include "impeller/compiler/uniform_sorter.h" using namespace spv; using namespace SPIRV_CROSS_NAMESPACE; @@ -220,13 +219,47 @@ bool CompilerSkSL::emit_uniform_resources() { bool emitted = false; // Output Uniform Constants (values, samplers, images, etc). - std::vector regular_uniforms = SortUniforms(&ir, this, SPIRType::Float); - std::vector shader_uniforms = - SortUniforms(&ir, this, SPIRType::SampledImage); - if (regular_uniforms.size() > 0 || shader_uniforms.size() > 0) { - emitted = true; + std::vector regular_uniforms; + std::vector shader_uniforms; + for (auto& id : ir.ids) { + if (id.get_type() == TypeVariable) { + auto& var = id.get(); + auto& type = get(var.basetype); + if (var.storage != StorageClassFunction && !is_hidden_variable(var) && + type.pointer && + (type.storage == StorageClassUniformConstant || + type.storage == StorageClassAtomicCounter)) { + // Separate out the uniforms that will be of SkSL 'shader' type since + // we need to make sure they are emitted only after the other uniforms. + if (type.basetype == SPIRType::SampledImage) { + shader_uniforms.push_back(var.self); + } else { + regular_uniforms.push_back(var.self); + } + emitted = true; + } + } } + // Sort uniforms by location. + auto compare_locations = [this](ID id1, ID id2) { + auto& flags1 = get_decoration_bitset(id1); + auto& flags2 = get_decoration_bitset(id2); + // Put the uniforms with no location after the ones that have a location. + if (!flags1.get(DecorationLocation)) { + return false; + } + if (!flags2.get(DecorationLocation)) { + return true; + } + // Sort in increasing order of location. + return get_decoration(id1, DecorationLocation) < + get_decoration(id2, DecorationLocation); + }; + std::sort(regular_uniforms.begin(), regular_uniforms.end(), + compare_locations); + std::sort(shader_uniforms.begin(), shader_uniforms.end(), compare_locations); + for (const auto& id : regular_uniforms) { auto& var = get(id); emit_uniform(var); diff --git a/engine/src/flutter/impeller/compiler/uniform_sorter.cc b/engine/src/flutter/impeller/compiler/uniform_sorter.cc deleted file mode 100644 index 5efe573093c..00000000000 --- a/engine/src/flutter/impeller/compiler/uniform_sorter.cc +++ /dev/null @@ -1,45 +0,0 @@ -// Copyright 2013 The Flutter Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "impeller/compiler/uniform_sorter.h" - -namespace impeller { - -std::vector SortUniforms( - const spirv_cross::ParsedIR* ir, - const spirv_cross::Compiler* compiler, - std::optional type_filter) { - // Sort the IR so that the uniforms are in declaration order. - std::vector uniforms; - ir->for_each_typed_id( - [&](uint32_t, const spirv_cross::SPIRVariable& var) { - if (var.storage != spv::StorageClassUniformConstant) { - return; - } - const auto type = compiler->get_type(var.basetype); - if (!type_filter.has_value() || type_filter.value() == type.basetype) { - uniforms.push_back(var.self); - } - }); - - auto compare_locations = [&ir](spirv_cross::ID id1, spirv_cross::ID id2) { - auto& flags1 = ir->get_decoration_bitset(id1); - auto& flags2 = ir->get_decoration_bitset(id2); - // Put the uniforms with no location after the ones that have a location. - if (!flags1.get(spv::Decoration::DecorationLocation)) { - return false; - } - if (!flags2.get(spv::Decoration::DecorationLocation)) { - return true; - } - // Sort in increasing order of location. - return ir->get_decoration(id1, spv::Decoration::DecorationLocation) < - ir->get_decoration(id2, spv::Decoration::DecorationLocation); - }; - std::sort(uniforms.begin(), uniforms.end(), compare_locations); - - return uniforms; -} - -} // namespace impeller diff --git a/engine/src/flutter/impeller/compiler/uniform_sorter.h b/engine/src/flutter/impeller/compiler/uniform_sorter.h deleted file mode 100644 index 2951891c695..00000000000 --- a/engine/src/flutter/impeller/compiler/uniform_sorter.h +++ /dev/null @@ -1,22 +0,0 @@ -// Copyright 2013 The Flutter Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#pragma once - -#include - -#include "impeller/compiler/compiler_backend.h" - -#include "spirv_msl.hpp" -#include "spirv_parser.hpp" - -namespace impeller { - -/// @brief Sorts uniform declarations in an IR according to decoration order. -std::vector SortUniforms( - const spirv_cross::ParsedIR* ir, - const spirv_cross::Compiler* compiler, - std::optional type_filter = std::nullopt); - -} // namespace impeller diff --git a/engine/src/flutter/impeller/entity/contents/runtime_effect_contents.cc b/engine/src/flutter/impeller/entity/contents/runtime_effect_contents.cc index dfb81ea9982..f36e0c9b21e 100644 --- a/engine/src/flutter/impeller/entity/contents/runtime_effect_contents.cc +++ b/engine/src/flutter/impeller/entity/contents/runtime_effect_contents.cc @@ -182,7 +182,7 @@ bool RuntimeEffectContents::Render(const ContentContext& renderer, ShaderUniformSlot uniform_slot; uniform_slot.name = uniform.name.c_str(); - uniform_slot.ext_res_0 = uniform.location; + uniform_slot.ext_res_0 = buffer_index; cmd.BindResource(ShaderStage::kFragment, uniform_slot, metadata, buffer_view); buffer_index++;