From 9760795bfb854733c4696fa9e7e04f788ea489a7 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Tue, 15 Oct 2019 18:14:52 -0400 Subject: [PATCH 1/8] gl_shader_decompiler: Avoid unnecessary copies of MetaImage MetaImage contains a std::vector, so copying here could result in unnecessary reallocations. Given the operation lives throughout the entire scope, this is safe to do. --- src/video_core/renderer_opengl/gl_shader_decompiler.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/video_core/renderer_opengl/gl_shader_decompiler.cpp b/src/video_core/renderer_opengl/gl_shader_decompiler.cpp index 6a610a3bca..3bd3d22443 100644 --- a/src/video_core/renderer_opengl/gl_shader_decompiler.cpp +++ b/src/video_core/renderer_opengl/gl_shader_decompiler.cpp @@ -1235,7 +1235,7 @@ private: std::string BuildImageValues(Operation operation) { constexpr std::array constructors{"uint", "uvec2", "uvec3", "uvec4"}; - const auto meta{std::get(operation.GetMeta())}; + const auto& meta{std::get(operation.GetMeta())}; const std::size_t values_count{meta.values.size()}; std::string expr = fmt::format("{}(", constructors.at(values_count - 1)); @@ -1780,14 +1780,14 @@ private: return {"0", Type::Int}; } - const auto meta{std::get(operation.GetMeta())}; + const auto& meta{std::get(operation.GetMeta())}; return {fmt::format("imageLoad({}, {}){}", GetImage(meta.image), BuildIntegerCoordinates(operation), GetSwizzle(meta.element)), Type::Uint}; } Expression ImageStore(Operation operation) { - const auto meta{std::get(operation.GetMeta())}; + const auto& meta{std::get(operation.GetMeta())}; code.AddLine("imageStore({}, {}, {});", GetImage(meta.image), BuildIntegerCoordinates(operation), BuildImageValues(operation)); return {}; @@ -1795,7 +1795,7 @@ private: template Expression AtomicImage(Operation operation) { - const auto meta{std::get(operation.GetMeta())}; + const auto& meta{std::get(operation.GetMeta())}; ASSERT(meta.values.size() == 1); return {fmt::format("imageAtomic{}({}, {}, {})", opname, GetImage(meta.image), From 67658dd6e84ca0ee04cdfd761aa1a7f5ac96cd42 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Tue, 15 Oct 2019 18:21:56 -0400 Subject: [PATCH 2/8] shader/node: std::move Meta instance within OperationNode constructor Allows usages of the constructor to avoid an unnecessary copy. --- src/video_core/shader/node.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/video_core/shader/node.h b/src/video_core/shader/node.h index 338bab17ca..447fb5c1d3 100644 --- a/src/video_core/shader/node.h +++ b/src/video_core/shader/node.h @@ -410,7 +410,7 @@ public: explicit OperationNode(OperationCode code) : OperationNode(code, Meta{}) {} explicit OperationNode(OperationCode code, Meta meta) - : OperationNode(code, meta, std::vector{}) {} + : OperationNode(code, std::move(meta), std::vector{}) {} explicit OperationNode(OperationCode code, std::vector operands) : OperationNode(code, Meta{}, std::move(operands)) {} From d1d7ce74d2e175d18c9eb385e202bca98cba08cd Mon Sep 17 00:00:00 2001 From: Lioncash Date: Tue, 15 Oct 2019 18:25:45 -0400 Subject: [PATCH 3/8] gl_shader_decompiler: Use std::holds_alternative within GenerateTexture() This only ever queries if the type exists within the variant, but doesn't actually do anything with the return value. We can just use std::holds_alternative for this use case. --- src/video_core/renderer_opengl/gl_shader_decompiler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/video_core/renderer_opengl/gl_shader_decompiler.cpp b/src/video_core/renderer_opengl/gl_shader_decompiler.cpp index 3bd3d22443..970cd0c766 100644 --- a/src/video_core/renderer_opengl/gl_shader_decompiler.cpp +++ b/src/video_core/renderer_opengl/gl_shader_decompiler.cpp @@ -1148,7 +1148,7 @@ private: for (const auto& variant : extras) { if (const auto argument = std::get_if(&variant)) { expr += GenerateTextureArgument(*argument); - } else if (std::get_if(&variant)) { + } else if (std::holds_alternative(variant)) { expr += GenerateTextureAoffi(meta->aoffi); } else { UNREACHABLE(); From b8a62adcf1034535353b4a1486a51a5775d209b5 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Tue, 15 Oct 2019 18:29:35 -0400 Subject: [PATCH 4/8] gl_shader_decompiler: Pass by reference to GenerateTextureArgument() Avoids an unnecessary atomic reference count increment and decrement. --- src/video_core/renderer_opengl/gl_shader_decompiler.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/video_core/renderer_opengl/gl_shader_decompiler.cpp b/src/video_core/renderer_opengl/gl_shader_decompiler.cpp index 970cd0c766..82228b55db 100644 --- a/src/video_core/renderer_opengl/gl_shader_decompiler.cpp +++ b/src/video_core/renderer_opengl/gl_shader_decompiler.cpp @@ -1158,8 +1158,8 @@ private: return expr + ')'; } - std::string GenerateTextureArgument(TextureArgument argument) { - const auto [type, operand] = argument; + std::string GenerateTextureArgument(const TextureArgument& argument) { + const auto& [type, operand] = argument; if (operand == nullptr) { return {}; } From 2f2ab9b5bcada1134d48f7a150463aa6f3d65a8b Mon Sep 17 00:00:00 2001 From: Lioncash Date: Tue, 15 Oct 2019 18:49:36 -0400 Subject: [PATCH 5/8] gl_shader_decompiler: Mark ASTDecompiler/ExprDecompiler parameters as const references where applicable These member functions don't actually modify the input parameter, so we can make this explicit with the use of const. --- .../renderer_opengl/gl_shader_decompiler.cpp | 42 +++++++++---------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/src/video_core/renderer_opengl/gl_shader_decompiler.cpp b/src/video_core/renderer_opengl/gl_shader_decompiler.cpp index 82228b55db..20dc7d3db1 100644 --- a/src/video_core/renderer_opengl/gl_shader_decompiler.cpp +++ b/src/video_core/renderer_opengl/gl_shader_decompiler.cpp @@ -2281,7 +2281,7 @@ class ExprDecompiler { public: explicit ExprDecompiler(GLSLDecompiler& decomp) : decomp{decomp} {} - void operator()(VideoCommon::Shader::ExprAnd& expr) { + void operator()(const ExprAnd& expr) { inner += "( "; std::visit(*this, *expr.operand1); inner += " && "; @@ -2289,7 +2289,7 @@ public: inner += ')'; } - void operator()(VideoCommon::Shader::ExprOr& expr) { + void operator()(const ExprOr& expr) { inner += "( "; std::visit(*this, *expr.operand1); inner += " || "; @@ -2297,17 +2297,17 @@ public: inner += ')'; } - void operator()(VideoCommon::Shader::ExprNot& expr) { + void operator()(const ExprNot& expr) { inner += '!'; std::visit(*this, *expr.operand1); } - void operator()(VideoCommon::Shader::ExprPredicate& expr) { + void operator()(const ExprPredicate& expr) { const auto pred = static_cast(expr.predicate); inner += decomp.GetPredicate(pred); } - void operator()(VideoCommon::Shader::ExprCondCode& expr) { + void operator()(const ExprCondCode& expr) { const Node cc = decomp.ir.GetConditionCode(expr.cc); std::string target; @@ -2329,11 +2329,11 @@ public: inner += target; } - void operator()(VideoCommon::Shader::ExprVar& expr) { + void operator()(const ExprVar& expr) { inner += GetFlowVariable(expr.var_index); } - void operator()(VideoCommon::Shader::ExprBoolean& expr) { + void operator()(const ExprBoolean& expr) { inner += expr.value ? "true" : "false"; } @@ -2350,7 +2350,7 @@ class ASTDecompiler { public: explicit ASTDecompiler(GLSLDecompiler& decomp) : decomp{decomp} {} - void operator()(VideoCommon::Shader::ASTProgram& ast) { + void operator()(const ASTProgram& ast) { ASTNode current = ast.nodes.GetFirst(); while (current) { Visit(current); @@ -2358,7 +2358,7 @@ public: } } - void operator()(VideoCommon::Shader::ASTIfThen& ast) { + void operator()(const ASTIfThen& ast) { ExprDecompiler expr_parser{decomp}; std::visit(expr_parser, *ast.condition); decomp.code.AddLine("if ({}) {{", expr_parser.GetResult()); @@ -2372,7 +2372,7 @@ public: decomp.code.AddLine("}}"); } - void operator()(VideoCommon::Shader::ASTIfElse& ast) { + void operator()(const ASTIfElse& ast) { decomp.code.AddLine("else {{"); decomp.code.scope++; ASTNode current = ast.nodes.GetFirst(); @@ -2384,29 +2384,29 @@ public: decomp.code.AddLine("}}"); } - void operator()(VideoCommon::Shader::ASTBlockEncoded& ast) { + void operator()([[maybe_unused]] const ASTBlockEncoded& ast) { UNREACHABLE(); } - void operator()(VideoCommon::Shader::ASTBlockDecoded& ast) { + void operator()(const ASTBlockDecoded& ast) { decomp.VisitBlock(ast.nodes); } - void operator()(VideoCommon::Shader::ASTVarSet& ast) { + void operator()(const ASTVarSet& ast) { ExprDecompiler expr_parser{decomp}; std::visit(expr_parser, *ast.condition); decomp.code.AddLine("{} = {};", GetFlowVariable(ast.index), expr_parser.GetResult()); } - void operator()(VideoCommon::Shader::ASTLabel& ast) { + void operator()(const ASTLabel& ast) { decomp.code.AddLine("// Label_{}:", ast.index); } - void operator()(VideoCommon::Shader::ASTGoto& ast) { + void operator()([[maybe_unused]] const ASTGoto& ast) { UNREACHABLE(); } - void operator()(VideoCommon::Shader::ASTDoWhile& ast) { + void operator()(const ASTDoWhile& ast) { ExprDecompiler expr_parser{decomp}; std::visit(expr_parser, *ast.condition); decomp.code.AddLine("do {{"); @@ -2420,7 +2420,7 @@ public: decomp.code.AddLine("}} while({});", expr_parser.GetResult()); } - void operator()(VideoCommon::Shader::ASTReturn& ast) { + void operator()(const ASTReturn& ast) { const bool is_true = VideoCommon::Shader::ExprIsTrue(ast.condition); if (!is_true) { ExprDecompiler expr_parser{decomp}; @@ -2440,7 +2440,7 @@ public: } } - void operator()(VideoCommon::Shader::ASTBreak& ast) { + void operator()(const ASTBreak& ast) { const bool is_true = VideoCommon::Shader::ExprIsTrue(ast.condition); if (!is_true) { ExprDecompiler expr_parser{decomp}; @@ -2455,7 +2455,7 @@ public: } } - void Visit(VideoCommon::Shader::ASTNode& node) { + void Visit(const ASTNode& node) { std::visit(*this, *node->GetInnerData()); } @@ -2468,9 +2468,9 @@ void GLSLDecompiler::DecompileAST() { for (u32 i = 0; i < num_flow_variables; i++) { code.AddLine("bool {} = false;", GetFlowVariable(i)); } + ASTDecompiler decompiler{*this}; - VideoCommon::Shader::ASTNode program = ir.GetASTProgram(); - decompiler.Visit(program); + decompiler.Visit(ir.GetASTProgram()); } } // Anonymous namespace From 04a1161354e9032e36d48d112e40dce2428b049d Mon Sep 17 00:00:00 2001 From: Lioncash Date: Tue, 15 Oct 2019 18:58:33 -0400 Subject: [PATCH 6/8] gl_shader_decompiler: Fold flow_var constant into GetFlowVariable() This is only ever used within this function, so we can narrow it's scope down. --- src/video_core/renderer_opengl/gl_shader_decompiler.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/video_core/renderer_opengl/gl_shader_decompiler.cpp b/src/video_core/renderer_opengl/gl_shader_decompiler.cpp index 20dc7d3db1..bd0c5c5d60 100644 --- a/src/video_core/renderer_opengl/gl_shader_decompiler.cpp +++ b/src/video_core/renderer_opengl/gl_shader_decompiler.cpp @@ -2271,10 +2271,8 @@ private: ShaderWriter code; }; -static constexpr std::string_view flow_var = "flow_var_"; - std::string GetFlowVariable(u32 i) { - return fmt::format("{}{}", flow_var, i); + return fmt::format("flow_var_{}", i); } class ExprDecompiler { From 67df3f7742187abb34d9a2eb4633334331c3318e Mon Sep 17 00:00:00 2001 From: Lioncash Date: Tue, 15 Oct 2019 19:00:20 -0400 Subject: [PATCH 7/8] gl_shader_decompiler: Use a std::string_view with GetDeclarationWithSuffix() This allows the function to be completely non-allocating for inputs of all sizes (i.e. there's no heap cost for an input to convert to a std::string_view). --- src/video_core/renderer_opengl/gl_shader_decompiler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/video_core/renderer_opengl/gl_shader_decompiler.cpp b/src/video_core/renderer_opengl/gl_shader_decompiler.cpp index bd0c5c5d60..2c50ed5386 100644 --- a/src/video_core/renderer_opengl/gl_shader_decompiler.cpp +++ b/src/video_core/renderer_opengl/gl_shader_decompiler.cpp @@ -2246,7 +2246,7 @@ private: code.AddLine("#ifdef SAMPLER_{}_IS_BUFFER", sampler.GetIndex()); } - std::string GetDeclarationWithSuffix(u32 index, const std::string& name) const { + std::string GetDeclarationWithSuffix(u32 index, std::string_view name) const { return fmt::format("{}_{}_{}", name, index, suffix); } From 4f16ce9294f8f77ecfcec35967e6f50caafdea0c Mon Sep 17 00:00:00 2001 From: Lioncash Date: Tue, 15 Oct 2019 19:02:57 -0400 Subject: [PATCH 8/8] gl_shader_decompiler: Make ExprDecompiler's GetResult() a const member function This is only ever used to read, but not write, the resulting string, so we can enforce this by making it a const member function. --- src/video_core/renderer_opengl/gl_shader_decompiler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/video_core/renderer_opengl/gl_shader_decompiler.cpp b/src/video_core/renderer_opengl/gl_shader_decompiler.cpp index 2c50ed5386..a3524a6a98 100644 --- a/src/video_core/renderer_opengl/gl_shader_decompiler.cpp +++ b/src/video_core/renderer_opengl/gl_shader_decompiler.cpp @@ -2335,7 +2335,7 @@ public: inner += expr.value ? "true" : "false"; } - std::string& GetResult() { + const std::string& GetResult() const { return inner; }