From e6eae4b815bf4bc480d62677fdf9bdbf5d6cba82 Mon Sep 17 00:00:00 2001 From: Fernando Sahmkow Date: Fri, 4 Oct 2019 17:23:16 -0400 Subject: [PATCH] Shader_ir: Address feedback --- .../renderer_opengl/gl_shader_decompiler.cpp | 12 +++-- .../renderer_opengl/gl_shader_gen.cpp | 13 +---- src/video_core/shader/ast.cpp | 48 +++---------------- src/video_core/shader/ast.h | 12 ++--- src/video_core/shader/control_flow.cpp | 2 +- src/video_core/shader/decode.cpp | 2 +- 6 files changed, 24 insertions(+), 65 deletions(-) diff --git a/src/video_core/renderer_opengl/gl_shader_decompiler.cpp b/src/video_core/renderer_opengl/gl_shader_decompiler.cpp index bff1067a4..6a610a3bc 100644 --- a/src/video_core/renderer_opengl/gl_shader_decompiler.cpp +++ b/src/video_core/renderer_opengl/gl_shader_decompiler.cpp @@ -2271,7 +2271,11 @@ private: ShaderWriter code; }; -const std::string flow_var = "flow_var_"; +static constexpr std::string_view flow_var = "flow_var_"; + +std::string GetFlowVariable(u32 i) { + return fmt::format("{}{}", flow_var, i); +} class ExprDecompiler { public: @@ -2326,7 +2330,7 @@ public: } void operator()(VideoCommon::Shader::ExprVar& expr) { - inner += flow_var + std::to_string(expr.var_index); + inner += GetFlowVariable(expr.var_index); } void operator()(VideoCommon::Shader::ExprBoolean& expr) { @@ -2391,7 +2395,7 @@ public: void operator()(VideoCommon::Shader::ASTVarSet& ast) { ExprDecompiler expr_parser{decomp}; std::visit(expr_parser, *ast.condition); - decomp.code.AddLine("{}{} = {};", flow_var, ast.index, expr_parser.GetResult()); + decomp.code.AddLine("{} = {};", GetFlowVariable(ast.index), expr_parser.GetResult()); } void operator()(VideoCommon::Shader::ASTLabel& ast) { @@ -2462,7 +2466,7 @@ private: void GLSLDecompiler::DecompileAST() { const u32 num_flow_variables = ir.GetASTNumVariables(); for (u32 i = 0; i < num_flow_variables; i++) { - code.AddLine("bool {}{} = false;", flow_var, i); + code.AddLine("bool {} = false;", GetFlowVariable(i)); } ASTDecompiler decompiler{*this}; VideoCommon::Shader::ASTNode program = ir.GetASTProgram(); diff --git a/src/video_core/renderer_opengl/gl_shader_gen.cpp b/src/video_core/renderer_opengl/gl_shader_gen.cpp index 72a49ebdc..b5a43e79e 100644 --- a/src/video_core/renderer_opengl/gl_shader_gen.cpp +++ b/src/video_core/renderer_opengl/gl_shader_gen.cpp @@ -19,6 +19,8 @@ using VideoCommon::Shader::ShaderIR; static constexpr u32 PROGRAM_OFFSET = 10; static constexpr u32 COMPUTE_OFFSET = 0; +static constexpr CompilerSettings settings{CompileDepth::NoFlowStack, true}; + ProgramResult GenerateVertexShader(const Device& device, const ShaderSetup& setup) { const std::string id = fmt::format("{:016x}", setup.program.unique_identifier); @@ -33,9 +35,6 @@ layout (std140, binding = EMULATION_UBO_BINDING) uniform vs_config { )"; - CompilerSettings settings; - settings.depth = CompileDepth::NoFlowStack; - const ShaderIR program_ir(setup.program.code, PROGRAM_OFFSET, setup.program.size_a, settings); const auto stage = setup.IsDualProgram() ? ProgramType::VertexA : ProgramType::VertexB; ProgramResult program = Decompile(device, program_ir, stage, "vertex"); @@ -86,9 +85,6 @@ layout (std140, binding = EMULATION_UBO_BINDING) uniform gs_config { )"; - CompilerSettings settings; - settings.depth = CompileDepth::NoFlowStack; - const ShaderIR program_ir(setup.program.code, PROGRAM_OFFSET, setup.program.size_a, settings); ProgramResult program = Decompile(device, program_ir, ProgramType::Geometry, "geometry"); out += program.first; @@ -123,8 +119,6 @@ layout (std140, binding = EMULATION_UBO_BINDING) uniform fs_config { }; )"; - CompilerSettings settings; - settings.depth = CompileDepth::NoFlowStack; const ShaderIR program_ir(setup.program.code, PROGRAM_OFFSET, setup.program.size_a, settings); ProgramResult program = Decompile(device, program_ir, ProgramType::Fragment, "fragment"); @@ -145,9 +139,6 @@ ProgramResult GenerateComputeShader(const Device& device, const ShaderSetup& set std::string out = "// Shader Unique Id: CS" + id + "\n\n"; out += GetCommonDeclarations(); - CompilerSettings settings; - settings.depth = CompileDepth::NoFlowStack; - const ShaderIR program_ir(setup.program.code, COMPUTE_OFFSET, setup.program.size_a, settings); ProgramResult program = Decompile(device, program_ir, ProgramType::Compute, "compute"); out += program.first; diff --git a/src/video_core/shader/ast.cpp b/src/video_core/shader/ast.cpp index c4548f0bc..2eb065c3d 100644 --- a/src/video_core/shader/ast.cpp +++ b/src/video_core/shader/ast.cpp @@ -376,7 +376,7 @@ void ASTManager::Init() { false_condition = MakeExpr(false); } -ASTManager::ASTManager(ASTManager&& other) +ASTManager::ASTManager(ASTManager&& other) noexcept : labels_map(std::move(other.labels_map)), labels_count{other.labels_count}, gotos(std::move(other.gotos)), labels(std::move(other.labels)), variables{other.variables}, program{other.program}, main_node{other.main_node}, false_condition{other.false_condition}, @@ -384,7 +384,7 @@ ASTManager::ASTManager(ASTManager&& other) other.main_node.reset(); } -ASTManager& ASTManager::operator=(ASTManager&& other) { +ASTManager& ASTManager::operator=(ASTManager&& other) noexcept { full_decompile = other.full_decompile; labels_map = std::move(other.labels_map); labels_count = other.labels_count; @@ -490,7 +490,7 @@ void ASTManager::Decompile() { it++; } if (full_decompile) { - for (const ASTNode label : labels) { + for (const ASTNode& label : labels) { auto& manager = label->GetManager(); manager.Remove(label); } @@ -500,12 +500,12 @@ void ASTManager::Decompile() { while (it != labels.end()) { bool can_remove = true; ASTNode label = *it; - for (const ASTNode goto_node : gotos) { + for (const ASTNode& goto_node : gotos) { const auto label_index = goto_node->GetGotoLabel(); if (!label_index) { return; } - ASTNode glabel = labels[*label_index]; + ASTNode& glabel = labels[*label_index]; if (glabel == label) { can_remove = false; break; @@ -543,40 +543,6 @@ bool ASTManager::IsBackwardsJump(ASTNode goto_node, ASTNode label_node) const { return false; } -ASTNode CommonParent(ASTNode first, ASTNode second) { - if (first->GetParent() == second->GetParent()) { - return first->GetParent(); - } - const u32 first_level = first->GetLevel(); - const u32 second_level = second->GetLevel(); - u32 min_level; - u32 max_level; - ASTNode max; - ASTNode min; - if (first_level > second_level) { - min_level = second_level; - min = second; - max_level = first_level; - max = first; - } else { - min_level = first_level; - min = first; - max_level = second_level; - max = second; - } - - while (max_level > min_level) { - max_level--; - max = max->GetParent(); - } - - while (min->GetParent() != max->GetParent()) { - min = min->GetParent(); - max = max->GetParent(); - } - return min->GetParent(); -} - bool ASTManager::IndirectlyRelated(ASTNode first, ASTNode second) { return !(first->GetParent() == second->GetParent() || DirectlyRelated(first, second)); } @@ -608,7 +574,7 @@ bool ASTManager::DirectlyRelated(ASTNode first, ASTNode second) { max = max->GetParent(); } - return (min->GetParent() == max->GetParent()); + return min->GetParent() == max->GetParent(); } void ASTManager::ShowCurrentState(std::string state) { @@ -617,7 +583,7 @@ void ASTManager::ShowCurrentState(std::string state) { } void ASTManager::SanityCheck() { - for (auto label : labels) { + for (auto& label : labels) { if (!label->GetParent()) { LOG_CRITICAL(HW_GPU, "Sanity Check Failed"); } diff --git a/src/video_core/shader/ast.h b/src/video_core/shader/ast.h index 8efd4c147..ba234138e 100644 --- a/src/video_core/shader/ast.h +++ b/src/video_core/shader/ast.h @@ -97,7 +97,7 @@ public: class ASTBlockDecoded { public: - explicit ASTBlockDecoded(NodeBlock& new_nodes) : nodes(std::move(new_nodes)) {} + explicit ASTBlockDecoded(NodeBlock&& new_nodes) : nodes(std::move(new_nodes)) {} NodeBlock nodes; }; @@ -255,8 +255,8 @@ public: return std::holds_alternative(data); } - void TransformBlockEncoded(NodeBlock& nodes) { - data = ASTBlockDecoded(nodes); + void TransformBlockEncoded(NodeBlock&& nodes) { + data = ASTBlockDecoded(std::move(nodes)); } bool IsLoop() const { @@ -304,8 +304,8 @@ public: ASTManager(const ASTManager& o) = delete; ASTManager& operator=(const ASTManager& other) = delete; - ASTManager(ASTManager&& other); - ASTManager& operator=(ASTManager&& other); + ASTManager(ASTManager&& other) noexcept; + ASTManager& operator=(ASTManager&& other) noexcept; void Init(); @@ -362,8 +362,6 @@ public: private: bool IsBackwardsJump(ASTNode goto_node, ASTNode label_node) const; - ASTNode CommonParent(ASTNode first, ASTNode second); - bool IndirectlyRelated(ASTNode first, ASTNode second); bool DirectlyRelated(ASTNode first, ASTNode second); diff --git a/src/video_core/shader/control_flow.cpp b/src/video_core/shader/control_flow.cpp index c2fa734e7..3c3a41ba6 100644 --- a/src/video_core/shader/control_flow.cpp +++ b/src/video_core/shader/control_flow.cpp @@ -58,7 +58,7 @@ struct BlockInfo { struct CFGRebuildState { explicit CFGRebuildState(const ProgramCode& program_code, const std::size_t program_size, const u32 start) - : program_code{program_code}, program_size{program_size}, start{start} {} + : start{start}, program_code{program_code}, program_size{program_size} {} u32 start{}; std::vector block_info{}; diff --git a/src/video_core/shader/decode.cpp b/src/video_core/shader/decode.cpp index 6d4359295..2626b1616 100644 --- a/src/video_core/shader/decode.cpp +++ b/src/video_core/shader/decode.cpp @@ -90,7 +90,7 @@ public: if (node->IsBlockEncoded()) { auto block = std::get_if(node->GetInnerData()); NodeBlock bb = ir.DecodeRange(block->start, block->end); - node->TransformBlockEncoded(bb); + node->TransformBlockEncoded(std::move(bb)); } }