Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] Patch the compiler to account for subpass inputs and PSO metadata. #45739

Merged
merged 1 commit into from
Sep 12, 2023
Merged

[Impeller] Patch the compiler to account for subpass inputs and PSO metadata. #45739

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion impeller/aiks/aiks_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1636,7 +1636,7 @@ TEST_P(AiksTest, ColorWheel) {
auto callback = [&](AiksContext& renderer) -> std::optional<Picture> {
// UI state.
static bool cache_the_wheel = true;
static int current_blend_index = 3;
static int current_blend_index = 14;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Keep default to SourceOver? Although advanced blends are more contentious, so maybe someone flipping through is more likely to notice a problem with Screen as the default.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, just thought defaulting to the advanced ones was more likely to throw up issues.

static float dst_alpha = 1;
static float src_alpha = 1;
static Color color0 = Color::Red();
Expand Down
12 changes: 11 additions & 1 deletion impeller/compiler/code_gen_template.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,20 +173,30 @@ std::move({{ arg.argument_name }}){% if not loop.is_last %}, {% endif %}
// ===========================================================================
// Metadata for Vulkan =======================================================
// ===========================================================================
static constexpr std::array<DescriptorSetLayout,{{length(buffers)+length(sampled_images)}}> kDescriptorSetLayouts{
static constexpr std::array<DescriptorSetLayout,{{length(buffers)+length(sampled_images)+length(subpass_inputs)}}> kDescriptorSetLayouts{
{% for buffer in buffers %}
DescriptorSetLayout{
{{buffer.binding}}, // binding = {{buffer.binding}}
{{buffer.input_attachment_index}}, // input_attachment_index = {{buffer.input_attachment_index}}
{{buffer.descriptor_type}}, // descriptor_type = {{buffer.descriptor_type}}
{{to_shader_stage(shader_stage)}}, // shader_stage = {{to_shader_stage(shader_stage)}}
},
{% endfor %}
{% for sampled_image in sampled_images %}
DescriptorSetLayout{
{{sampled_image.binding}}, // binding = {{sampled_image.binding}}
{{sampled_image.input_attachment_index}}, // input_attachment_index = {{sampled_image.input_attachment_index}}
{{sampled_image.descriptor_type}}, // descriptor_type = {{sampled_image.descriptor_type}}
{{to_shader_stage(shader_stage)}}, // shader_stage = {{to_shader_stage(shader_stage)}}
},
{% endfor %}
{% for subpass_input in subpass_inputs %}
DescriptorSetLayout{
{{subpass_input.binding}}, // binding = {{subpass_input.binding}}
{{subpass_input.input_attachment_index}}, // input_attachment_index = {{subpass_input.input_attachment_index}}
{{subpass_input.descriptor_type}}, // descriptor_type = {{subpass_input.descriptor_type}}
{{to_shader_stage(shader_stage)}}, // shader_stage = {{to_shader_stage(shader_stage)}}
},
{% endfor %}
};

Expand Down
17 changes: 8 additions & 9 deletions impeller/compiler/compiler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,15 +115,13 @@ static CompilerBackend CreateMSLCompiler(
static CompilerBackend CreateVulkanCompiler(
const spirv_cross::ParsedIR& ir,
const SourceOptions& source_options) {
// TODO(dnfield): It seems like what we'd want is a CompilerGLSL with
// vulkan_semantics set to true, but that has regressed some things on GLES
// somehow. In the mean time, go back to using CompilerMSL, but set the Metal
// Language version to something really high so that we don't get weird
// complaints about using Metal features while trying to build Vulkan shaders.
// https://github.com/flutter/flutter/issues/123795
return CreateMSLCompiler(
ir, source_options,
spirv_cross::CompilerMSL::Options::make_msl_version(3, 0, 0));
auto vk_compiler = std::make_shared<spirv_cross::CompilerGLSL>(ir);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know how this change had previously broken GLES and how this changed fixed it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original issue was light on details but there is now a separate CreateGLSLCompiler which is probably what's used. So this change shouldn't affect GLSL at all. I believe earlier there used to be a common compiler instance creation and I can believe was hard to configure just right for one backend without causing unintended consequences elsewhere. But again, the issue was light on details so its hard to say. I'll keep an eye out for repercussions.

spirv_cross::CompilerGLSL::Options sl_options;
sl_options.vulkan_semantics = true;
sl_options.force_zero_initialized_variables = true;
sl_options.es = false;
vk_compiler->set_common_options(sl_options);
return CompilerBackend(vk_compiler);
}

static CompilerBackend CreateGLSLCompiler(const spirv_cross::ParsedIR& ir,
Expand Down Expand Up @@ -218,6 +216,7 @@ static CompilerBackend CreateCompiler(const spirv_cross::ParsedIR& ir,
break;
case TargetPlatform::kSkSL:
compiler = CreateSkSLCompiler(ir, source_options);
break;
}
if (!compiler) {
return {};
Expand Down
5 changes: 3 additions & 2 deletions impeller/compiler/compiler_backend.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

#include "impeller/compiler/compiler_backend.h"

#include <limits>

#include "impeller/base/comparable.h"

namespace impeller {
Expand Down Expand Up @@ -44,8 +46,7 @@ uint32_t CompilerBackend::GetExtendedMSLResourceBinding(
if (auto compiler = GetGLSLCompiler()) {
return compiler->get_decoration(id, spv::Decoration::DecorationBinding);
}
const auto kOOBIndex = static_cast<uint32_t>(-1);
return kOOBIndex;
return std::numeric_limits<uint32_t>::max();
}

const spirv_cross::Compiler* CompilerBackend::GetCompiler() const {
Expand Down
30 changes: 27 additions & 3 deletions impeller/compiler/reflector.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "impeller/compiler/reflector.h"

#include <atomic>
#include <limits>
#include <optional>
#include <set>
#include <sstream>
Expand Down Expand Up @@ -216,9 +217,9 @@ std::optional<nlohmann::json> Reflector::GenerateTemplateArguments() const {
if (auto storage_buffers_json =
ReflectResources(shader_resources.storage_buffers);
storage_buffers_json.has_value()) {
for (auto uniform_buffer : storage_buffers_json.value()) {
uniform_buffer["descriptor_type"] = "DescriptorType::kStorageBuffer";
buffers.emplace_back(std::move(uniform_buffer));
for (auto storage_buffer : storage_buffers_json.value()) {
storage_buffer["descriptor_type"] = "DescriptorType::kStorageBuffer";
buffers.emplace_back(std::move(storage_buffer));
}
} else {
return std::nullopt;
Expand Down Expand Up @@ -261,6 +262,19 @@ std::optional<nlohmann::json> Reflector::GenerateTemplateArguments() const {
}
}

{
if (auto inputs = ReflectResources(shader_resources.subpass_inputs);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, convenient!

inputs.has_value()) {
auto& subpass_inputs = root["subpass_inputs"] = nlohmann::json::array_t{};
for (auto input : inputs.value()) {
input["descriptor_type"] = "DescriptorType::kSubpassInput";
subpass_inputs.emplace_back(std::move(input));
}
} else {
return std::nullopt;
}
}

if (auto stage_outputs = ReflectResources(shader_resources.stage_outputs);
stage_outputs.has_value()) {
root["stage_outputs"] = std::move(stage_outputs.value());
Expand Down Expand Up @@ -431,6 +445,14 @@ std::vector<size_t> Reflector::ComputeOffsets(
return offsets;
}

static uint32_t GetResourceDecorationIfPresent(const CompilerBackend& compiler,
spirv_cross::ID id,
spv::Decoration decoration) {
return compiler->has_decoration(id, decoration)
? compiler->get_decoration(id, decoration)
: std::numeric_limits<uint32_t>::max();
}

std::optional<nlohmann::json::object_t> Reflector::ReflectResource(
const spirv_cross::Resource& resource,
std::optional<size_t> offset) const {
Expand All @@ -445,6 +467,8 @@ std::optional<nlohmann::json::object_t> Reflector::ReflectResource(
resource.id, spv::Decoration::DecorationDescriptorSet);
result["location"] = compiler_->get_decoration(
resource.id, spv::Decoration::DecorationLocation);
result["input_attachment_index"] = GetResourceDecorationIfPresent(
compiler_, resource.id, spv::Decoration::DecorationInputAttachmentIndex);
result["index"] =
compiler_->get_decoration(resource.id, spv::Decoration::DecorationIndex);
result["ext_res_0"] = compiler_.GetExtendedMSLResourceBinding(
Expand Down
10 changes: 10 additions & 0 deletions impeller/core/shader_types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#pragma once

#include <cstddef>
#include <limits>
#include <optional>
#include <string_view>
#include <vector>
Expand Down Expand Up @@ -144,12 +145,21 @@ enum class DescriptorType {
kSampledImage,
kImage,
kSampler,
kSubpassInput,
};

struct DescriptorSetLayout {
uint32_t binding;
uint32_t input_attachment_index;
DescriptorType descriptor_type;
ShaderStage shader_stage;

constexpr std::optional<uint32_t> GetInputAttachmentIndexIfValid() const {
if (std::numeric_limits<uint32_t>::max() == input_attachment_index) {
return std::nullopt;
}
return input_attachment_index;
}
};

template <size_t Size>
Expand Down
4 changes: 4 additions & 0 deletions impeller/playground/backend/vulkan/playground_impl_vk.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "flutter/fml/logging.h"
#include "flutter/fml/mapping.h"
#include "impeller/entity/vk/entity_shaders_vk.h"
#include "impeller/entity/vk/framebuffer_blend_shaders_vk.h"
#include "impeller/entity/vk/modern_shaders_vk.h"
#include "impeller/fixtures/vk/fixtures_shaders_vk.h"
#include "impeller/playground/imgui/vk/imgui_shaders_vk.h"
Expand All @@ -33,6 +34,9 @@ ShaderLibraryMappingsForPlayground() {
impeller_entity_shaders_vk_length),
std::make_shared<fml::NonOwnedMapping>(impeller_modern_shaders_vk_data,
impeller_modern_shaders_vk_length),
std::make_shared<fml::NonOwnedMapping>(
impeller_framebuffer_blend_shaders_vk_data,
impeller_framebuffer_blend_shaders_vk_length),
std::make_shared<fml::NonOwnedMapping>(
impeller_fixtures_shaders_vk_data,
impeller_fixtures_shaders_vk_length),
Expand Down
4 changes: 3 additions & 1 deletion impeller/renderer/backend/vulkan/descriptor_pool_vk.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ static vk::UniqueDescriptorPool CreatePool(const vk::Device& device,
std::vector<vk::DescriptorPoolSize> pools = {
{vk::DescriptorType::eCombinedImageSampler, pool_count},
{vk::DescriptorType::eUniformBuffer, pool_count},
{vk::DescriptorType::eStorageBuffer, pool_count}};
{vk::DescriptorType::eStorageBuffer, pool_count},
{vk::DescriptorType::eInputAttachment, pool_count},
};

vk::DescriptorPoolCreateInfo pool_info;
pool_info.setMaxSets(pools.size() * pool_count);
Expand Down
8 changes: 2 additions & 6 deletions impeller/renderer/backend/vulkan/formats_vk.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,21 +274,17 @@ constexpr vk::DescriptorType ToVKDescriptorType(DescriptorType type) {
switch (type) {
case DescriptorType::kSampledImage:
return vk::DescriptorType::eCombinedImageSampler;
break;
case DescriptorType::kUniformBuffer:
return vk::DescriptorType::eUniformBuffer;
break;
case DescriptorType::kStorageBuffer:
return vk::DescriptorType::eStorageBuffer;
break;
case DescriptorType::kImage:
return vk::DescriptorType::eSampledImage;
break;
case DescriptorType::kSampler:
return vk::DescriptorType::eSampler;
break;
case DescriptorType::kSubpassInput:
return vk::DescriptorType::eInputAttachment;
}

FML_UNREACHABLE();
}

Expand Down
2 changes: 1 addition & 1 deletion shell/platform/embedder/embedder_surface_metal_impeller.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
#include "flutter/fml/synchronization/sync_switch.h"
#include "flutter/shell/gpu/gpu_surface_metal_delegate.h"
#include "flutter/shell/gpu/gpu_surface_metal_impeller.h"
#import "flutter/shell/platform/darwin/graphics/FlutterDarwinContextMetalImpeller.h"
#include "flutter/shell/platform/darwin/graphics/FlutterDarwinContextMetalImpeller.h"
#include "impeller/entity/mtl/entity_shaders.h"
#include "impeller/entity/mtl/framebuffer_blend_shaders.h"
#include "impeller/entity/mtl/modern_shaders.h"
Expand Down