From 081530686c37afd668b389103a97e573399a83a1 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Thu, 17 Oct 2019 19:54:17 -0400 Subject: [PATCH 1/7] video_core/shader/ast: Make use of fmt where applicable Makes a few strings nicer to read and also eliminates a bit of string churn with operator+. --- src/video_core/shader/ast.cpp | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/video_core/shader/ast.cpp b/src/video_core/shader/ast.cpp index 436d45f4b..0be048044 100644 --- a/src/video_core/shader/ast.cpp +++ b/src/video_core/shader/ast.cpp @@ -4,6 +4,8 @@ #include +#include + #include "common/assert.h" #include "common/common_types.h" #include "video_core/shader/ast.h" @@ -249,7 +251,7 @@ public: void operator()(const ASTIfThen& ast) { ExprPrinter expr_parser{}; std::visit(expr_parser, *ast.condition); - inner += Ident() + "if (" + expr_parser.GetResult() + ") {\n"; + inner += fmt::format("{}if ({}) {{\n", Ident(), expr_parser.GetResult()); scope++; ASTNode current = ast.nodes.GetFirst(); while (current) { @@ -257,7 +259,7 @@ public: current = current->GetNext(); } scope--; - inner += Ident() + "}\n"; + inner += fmt::format("{}}}\n", Ident()); } void operator()(const ASTIfElse& ast) { @@ -273,8 +275,7 @@ public: } void operator()(const ASTBlockEncoded& ast) { - inner += Ident() + "Block(" + std::to_string(ast.start) + ", " + std::to_string(ast.end) + - ");\n"; + inner += fmt::format("{}Block({}, {});\n", Ident(), ast.start, ast.end); } void operator()(const ASTBlockDecoded& ast) { @@ -284,25 +285,24 @@ public: void operator()(const ASTVarSet& ast) { ExprPrinter expr_parser{}; std::visit(expr_parser, *ast.condition); - inner += - Ident() + "V" + std::to_string(ast.index) + " := " + expr_parser.GetResult() + ";\n"; + inner += fmt::format("{}V{} := {};\n", Ident(), ast.index, expr_parser.GetResult()); } void operator()(const ASTLabel& ast) { - inner += "Label_" + std::to_string(ast.index) + ":\n"; + inner += fmt::format("Label_{}:\n", ast.index); } void operator()(const ASTGoto& ast) { ExprPrinter expr_parser{}; std::visit(expr_parser, *ast.condition); - inner += Ident() + "(" + expr_parser.GetResult() + ") -> goto Label_" + - std::to_string(ast.label) + ";\n"; + inner += + fmt::format("{}({}) -> goto Label_{};\n", Ident(), expr_parser.GetResult(), ast.label); } void operator()(const ASTDoWhile& ast) { ExprPrinter expr_parser{}; std::visit(expr_parser, *ast.condition); - inner += Ident() + "do {\n"; + inner += fmt::format("{}do {{\n", Ident()); scope++; ASTNode current = ast.nodes.GetFirst(); while (current) { @@ -310,20 +310,20 @@ public: current = current->GetNext(); } scope--; - inner += Ident() + "} while (" + expr_parser.GetResult() + ");\n"; + inner += fmt::format("{}}} while ({});\n", Ident(), expr_parser.GetResult()); } void operator()(const ASTReturn& ast) { ExprPrinter expr_parser{}; std::visit(expr_parser, *ast.condition); - inner += Ident() + "(" + expr_parser.GetResult() + ") -> " + - (ast.kills ? "discard" : "exit") + ";\n"; + inner += fmt::format("{}({}) -> {};\n", Ident(), expr_parser.GetResult(), + ast.kills ? "discard" : "exit"); } void operator()(const ASTBreak& ast) { ExprPrinter expr_parser{}; std::visit(expr_parser, *ast.condition); - inner += Ident() + "(" + expr_parser.GetResult() + ") -> break;\n"; + inner += fmt::format("{}({}) -> break;\n", Ident(), expr_parser.GetResult()); } std::string& Ident() { From 7f6a8a33d4deefea1c6360d8010893286664175e Mon Sep 17 00:00:00 2001 From: Lioncash Date: Thu, 17 Oct 2019 20:05:25 -0400 Subject: [PATCH 2/7] video_core/shader/ast: Rename Ident() to Indent() This can be confusing, given "ident" is generally used as a shorthand for "identifier". --- src/video_core/shader/ast.cpp | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/video_core/shader/ast.cpp b/src/video_core/shader/ast.cpp index 0be048044..ea121e9d5 100644 --- a/src/video_core/shader/ast.cpp +++ b/src/video_core/shader/ast.cpp @@ -251,7 +251,7 @@ public: void operator()(const ASTIfThen& ast) { ExprPrinter expr_parser{}; std::visit(expr_parser, *ast.condition); - inner += fmt::format("{}if ({}) {{\n", Ident(), expr_parser.GetResult()); + inner += fmt::format("{}if ({}) {{\n", Indent(), expr_parser.GetResult()); scope++; ASTNode current = ast.nodes.GetFirst(); while (current) { @@ -259,11 +259,11 @@ public: current = current->GetNext(); } scope--; - inner += fmt::format("{}}}\n", Ident()); + inner += fmt::format("{}}}\n", Indent()); } void operator()(const ASTIfElse& ast) { - inner += Ident() + "else {\n"; + inner += Indent() + "else {\n"; scope++; ASTNode current = ast.nodes.GetFirst(); while (current) { @@ -271,21 +271,21 @@ public: current = current->GetNext(); } scope--; - inner += Ident() + "}\n"; + inner += Indent() + "}\n"; } void operator()(const ASTBlockEncoded& ast) { - inner += fmt::format("{}Block({}, {});\n", Ident(), ast.start, ast.end); + inner += fmt::format("{}Block({}, {});\n", Indent(), ast.start, ast.end); } void operator()(const ASTBlockDecoded& ast) { - inner += Ident() + "Block;\n"; + inner += Indent() + "Block;\n"; } void operator()(const ASTVarSet& ast) { ExprPrinter expr_parser{}; std::visit(expr_parser, *ast.condition); - inner += fmt::format("{}V{} := {};\n", Ident(), ast.index, expr_parser.GetResult()); + inner += fmt::format("{}V{} := {};\n", Indent(), ast.index, expr_parser.GetResult()); } void operator()(const ASTLabel& ast) { @@ -296,13 +296,13 @@ public: ExprPrinter expr_parser{}; std::visit(expr_parser, *ast.condition); inner += - fmt::format("{}({}) -> goto Label_{};\n", Ident(), expr_parser.GetResult(), ast.label); + fmt::format("{}({}) -> goto Label_{};\n", Indent(), expr_parser.GetResult(), ast.label); } void operator()(const ASTDoWhile& ast) { ExprPrinter expr_parser{}; std::visit(expr_parser, *ast.condition); - inner += fmt::format("{}do {{\n", Ident()); + inner += fmt::format("{}do {{\n", Indent()); scope++; ASTNode current = ast.nodes.GetFirst(); while (current) { @@ -310,23 +310,23 @@ public: current = current->GetNext(); } scope--; - inner += fmt::format("{}}} while ({});\n", Ident(), expr_parser.GetResult()); + inner += fmt::format("{}}} while ({});\n", Indent(), expr_parser.GetResult()); } void operator()(const ASTReturn& ast) { ExprPrinter expr_parser{}; std::visit(expr_parser, *ast.condition); - inner += fmt::format("{}({}) -> {};\n", Ident(), expr_parser.GetResult(), + inner += fmt::format("{}({}) -> {};\n", Indent(), expr_parser.GetResult(), ast.kills ? "discard" : "exit"); } void operator()(const ASTBreak& ast) { ExprPrinter expr_parser{}; std::visit(expr_parser, *ast.condition); - inner += fmt::format("{}({}) -> break;\n", Ident(), expr_parser.GetResult()); + inner += fmt::format("{}({}) -> break;\n", Indent(), expr_parser.GetResult()); } - std::string& Ident() { + std::string& Indent() { if (memo_scope == scope) { return tabs_memo; } From 15d177a6ac81745f473e0807faf2da634dfb0fcd Mon Sep 17 00:00:00 2001 From: Lioncash Date: Thu, 17 Oct 2019 20:06:53 -0400 Subject: [PATCH 3/7] video_core/shader/ast: Make Indent() private It's never used outside of this class, so we can narrow its scope down. --- src/video_core/shader/ast.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/video_core/shader/ast.cpp b/src/video_core/shader/ast.cpp index ea121e9d5..f409706cc 100644 --- a/src/video_core/shader/ast.cpp +++ b/src/video_core/shader/ast.cpp @@ -326,15 +326,6 @@ public: inner += fmt::format("{}({}) -> break;\n", Indent(), expr_parser.GetResult()); } - std::string& Indent() { - if (memo_scope == scope) { - return tabs_memo; - } - tabs_memo = tabs.substr(0, scope * 2); - memo_scope = scope; - return tabs_memo; - } - void Visit(ASTNode& node) { std::visit(*this, *node->GetInnerData()); } @@ -344,6 +335,15 @@ public: } private: + std::string& Indent() { + if (memo_scope == scope) { + return tabs_memo; + } + tabs_memo = tabs.substr(0, scope * 2); + memo_scope = scope; + return tabs_memo; + } + std::string inner{}; u32 scope{}; From a2eccbf075ea686cb54d7ebce719cf9ad6cf551b Mon Sep 17 00:00:00 2001 From: Lioncash Date: Thu, 17 Oct 2019 20:07:57 -0400 Subject: [PATCH 4/7] video_core/shader/ast: Make Indent() return a string_view The returned string is simply a substring of our constexpr tabs string_view, so we can just use a string_view here as well, since the original string_view is guaranteed to always exist. Now the function is fully non-allocating. --- src/video_core/shader/ast.cpp | 38 ++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/src/video_core/shader/ast.cpp b/src/video_core/shader/ast.cpp index f409706cc..9ef1d0a78 100644 --- a/src/video_core/shader/ast.cpp +++ b/src/video_core/shader/ast.cpp @@ -3,6 +3,7 @@ // Refer to the license.txt file included. #include +#include #include @@ -263,7 +264,9 @@ public: } void operator()(const ASTIfElse& ast) { - inner += Indent() + "else {\n"; + inner += Indent(); + inner += "else {\n"; + scope++; ASTNode current = ast.nodes.GetFirst(); while (current) { @@ -271,15 +274,18 @@ public: current = current->GetNext(); } scope--; - inner += Indent() + "}\n"; + + inner += Indent(); + inner += "}\n"; } void operator()(const ASTBlockEncoded& ast) { inner += fmt::format("{}Block({}, {});\n", Indent(), ast.start, ast.end); } - void operator()(const ASTBlockDecoded& ast) { - inner += Indent() + "Block;\n"; + void operator()([[maybe_unused]] const ASTBlockDecoded& ast) { + inner += Indent(); + inner += "Block;\n"; } void operator()(const ASTVarSet& ast) { @@ -335,22 +341,26 @@ public: } private: - std::string& Indent() { - if (memo_scope == scope) { - return tabs_memo; + std::string_view Indent() { + if (space_segment_scope == scope) { + return space_segment; } - tabs_memo = tabs.substr(0, scope * 2); - memo_scope = scope; - return tabs_memo; + + // Ensure that we don't exceed our view. + ASSERT(scope * 2 < spaces.size()); + + space_segment = spaces.substr(0, scope * 2); + space_segment_scope = scope; + return space_segment; } std::string inner{}; + std::string_view space_segment; + u32 scope{}; + u32 space_segment_scope{}; - std::string tabs_memo{}; - u32 memo_scope{}; - - static constexpr std::string_view tabs{" "}; + static constexpr std::string_view spaces{" "}; }; std::string ASTManager::Print() { From 7831e86c34e45cdb8608ec47823b64ff88d09ac0 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Thu, 17 Oct 2019 20:39:33 -0400 Subject: [PATCH 5/7] video_core/shader/ast: Make ExprPrinter members private This member already has an accessor, so there's no need for it to be public. --- src/video_core/shader/ast.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/video_core/shader/ast.cpp b/src/video_core/shader/ast.cpp index 9ef1d0a78..c7d471d50 100644 --- a/src/video_core/shader/ast.cpp +++ b/src/video_core/shader/ast.cpp @@ -232,7 +232,8 @@ public: return inner; } - std::string inner{}; +private: + std::string inner; }; class ASTPrinter { From 222f4b45ebd545375b7befb84cfff81ae024e691 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Thu, 17 Oct 2019 20:56:36 -0400 Subject: [PATCH 6/7] video_core/shader/ast: Make ASTManager::Print a const member function Given all visiting functions never modify the nodes, we can trivially make this a const member function. --- src/video_core/shader/ast.cpp | 4 ++-- src/video_core/shader/ast.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/video_core/shader/ast.cpp b/src/video_core/shader/ast.cpp index c7d471d50..784f8f69f 100644 --- a/src/video_core/shader/ast.cpp +++ b/src/video_core/shader/ast.cpp @@ -333,7 +333,7 @@ public: inner += fmt::format("{}({}) -> break;\n", Indent(), expr_parser.GetResult()); } - void Visit(ASTNode& node) { + void Visit(const ASTNode& node) { std::visit(*this, *node->GetInnerData()); } @@ -364,7 +364,7 @@ private: static constexpr std::string_view spaces{" "}; }; -std::string ASTManager::Print() { +std::string ASTManager::Print() const { ASTPrinter printer{}; printer.Visit(main_node); return printer.GetResult(); diff --git a/src/video_core/shader/ast.h b/src/video_core/shader/ast.h index d7bf11821..8183ffa3f 100644 --- a/src/video_core/shader/ast.h +++ b/src/video_core/shader/ast.h @@ -328,7 +328,7 @@ public: void InsertReturn(Expr condition, bool kills); - std::string Print(); + std::string Print() const; void Decompile(); From 074b38b7a918a52f9085490f0da44b6e0dc23639 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Thu, 17 Oct 2019 20:59:46 -0400 Subject: [PATCH 7/7] video_core/shader/ast: Make ShowCurrentState() and SanityCheck() const member functions These can also trivially be made const member functions, with the addition of a few consts. --- src/video_core/shader/ast.cpp | 6 +++--- src/video_core/shader/ast.h | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/video_core/shader/ast.cpp b/src/video_core/shader/ast.cpp index 784f8f69f..e43aecc18 100644 --- a/src/video_core/shader/ast.cpp +++ b/src/video_core/shader/ast.cpp @@ -560,13 +560,13 @@ bool ASTManager::DirectlyRelated(const ASTNode& first, const ASTNode& second) co return min->GetParent() == max->GetParent(); } -void ASTManager::ShowCurrentState(std::string_view state) { +void ASTManager::ShowCurrentState(std::string_view state) const { LOG_CRITICAL(HW_GPU, "\nState {}:\n\n{}\n", state, Print()); SanityCheck(); } -void ASTManager::SanityCheck() { - for (auto& label : labels) { +void ASTManager::SanityCheck() const { + for (const 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 8183ffa3f..a2f0044ba 100644 --- a/src/video_core/shader/ast.h +++ b/src/video_core/shader/ast.h @@ -332,9 +332,9 @@ public: void Decompile(); - void ShowCurrentState(std::string_view state); + void ShowCurrentState(std::string_view state) const; - void SanityCheck(); + void SanityCheck() const; void Clear();