From 8e0c80f26914552e11144bd92dae726b66c3739d Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 5 Oct 2019 08:06:44 -0400 Subject: [PATCH 01/11] video_core/ast: Supply const accessors for data where applicable Provides const equivalents of data accessors for use within const contexts. --- src/video_core/shader/ast.cpp | 66 ++++++++++++++++------------------- src/video_core/shader/ast.h | 12 +++++-- 2 files changed, 41 insertions(+), 37 deletions(-) diff --git a/src/video_core/shader/ast.cpp b/src/video_core/shader/ast.cpp index 2eb065c3d..87c722682 100644 --- a/src/video_core/shader/ast.cpp +++ b/src/video_core/shader/ast.cpp @@ -185,9 +185,7 @@ void ASTZipper::Remove(const ASTNode node) { class ExprPrinter final { public: - ExprPrinter() = default; - - void operator()(ExprAnd const& expr) { + void operator()(const ExprAnd& expr) { inner += "( "; std::visit(*this, *expr.operand1); inner += " && "; @@ -195,7 +193,7 @@ public: inner += ')'; } - void operator()(ExprOr const& expr) { + void operator()(const ExprOr& expr) { inner += "( "; std::visit(*this, *expr.operand1); inner += " || "; @@ -203,29 +201,29 @@ public: inner += ')'; } - void operator()(ExprNot const& expr) { + void operator()(const ExprNot& expr) { inner += "!"; std::visit(*this, *expr.operand1); } - void operator()(ExprPredicate const& expr) { + void operator()(const ExprPredicate& expr) { inner += "P" + std::to_string(expr.predicate); } - void operator()(ExprCondCode const& expr) { + void operator()(const ExprCondCode& expr) { u32 cc = static_cast(expr.cc); inner += "CC" + std::to_string(cc); } - void operator()(ExprVar const& expr) { + void operator()(const ExprVar& expr) { inner += "V" + std::to_string(expr.var_index); } - void operator()(ExprBoolean const& expr) { + void operator()(const ExprBoolean& expr) { inner += expr.value ? "true" : "false"; } - std::string& GetResult() { + const std::string& GetResult() const { return inner; } @@ -234,9 +232,7 @@ public: class ASTPrinter { public: - ASTPrinter() = default; - - void operator()(ASTProgram& ast) { + void operator()(const ASTProgram& ast) { scope++; inner += "program {\n"; ASTNode current = ast.nodes.GetFirst(); @@ -248,7 +244,7 @@ public: scope--; } - void operator()(ASTIfThen& ast) { + void operator()(const ASTIfThen& ast) { ExprPrinter expr_parser{}; std::visit(expr_parser, *ast.condition); inner += Ident() + "if (" + expr_parser.GetResult() + ") {\n"; @@ -262,7 +258,7 @@ public: inner += Ident() + "}\n"; } - void operator()(ASTIfElse& ast) { + void operator()(const ASTIfElse& ast) { inner += Ident() + "else {\n"; scope++; ASTNode current = ast.nodes.GetFirst(); @@ -274,34 +270,34 @@ public: inner += Ident() + "}\n"; } - void operator()(ASTBlockEncoded& ast) { + void operator()(const ASTBlockEncoded& ast) { inner += Ident() + "Block(" + std::to_string(ast.start) + ", " + std::to_string(ast.end) + ");\n"; } - void operator()(ASTBlockDecoded& ast) { + void operator()(const ASTBlockDecoded& ast) { inner += Ident() + "Block;\n"; } - void operator()(ASTVarSet& ast) { + 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"; } - void operator()(ASTLabel& ast) { + void operator()(const ASTLabel& ast) { inner += "Label_" + std::to_string(ast.index) + ":\n"; } - void operator()(ASTGoto& ast) { + 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"; } - void operator()(ASTDoWhile& ast) { + void operator()(const ASTDoWhile& ast) { ExprPrinter expr_parser{}; std::visit(expr_parser, *ast.condition); inner += Ident() + "do {\n"; @@ -315,14 +311,14 @@ public: inner += Ident() + "} while (" + expr_parser.GetResult() + ");\n"; } - void operator()(ASTReturn& ast) { + void operator()(const ASTReturn& ast) { ExprPrinter expr_parser{}; std::visit(expr_parser, *ast.condition); inner += Ident() + "(" + expr_parser.GetResult() + ") -> " + (ast.kills ? "discard" : "exit") + ";\n"; } - void operator()(ASTBreak& ast) { + void operator()(const ASTBreak& ast) { ExprPrinter expr_parser{}; std::visit(expr_parser, *ast.condition); inner += Ident() + "(" + expr_parser.GetResult() + ") -> break;\n"; @@ -341,7 +337,7 @@ public: std::visit(*this, *node->GetInnerData()); } - std::string& GetResult() { + const std::string& GetResult() const { return inner; } @@ -696,7 +692,7 @@ class ASTClearer { public: ASTClearer() = default; - void operator()(ASTProgram& ast) { + void operator()(const ASTProgram& ast) { ASTNode current = ast.nodes.GetFirst(); while (current) { Visit(current); @@ -704,7 +700,7 @@ public: } } - void operator()(ASTIfThen& ast) { + void operator()(const ASTIfThen& ast) { ASTNode current = ast.nodes.GetFirst(); while (current) { Visit(current); @@ -712,7 +708,7 @@ public: } } - void operator()(ASTIfElse& ast) { + void operator()(const ASTIfElse& ast) { ASTNode current = ast.nodes.GetFirst(); while (current) { Visit(current); @@ -720,19 +716,19 @@ public: } } - void operator()(ASTBlockEncoded& ast) {} + void operator()([[maybe_unused]] const ASTBlockEncoded& ast) {} void operator()(ASTBlockDecoded& ast) { ast.nodes.clear(); } - void operator()(ASTVarSet& ast) {} + void operator()([[maybe_unused]] const ASTVarSet& ast) {} - void operator()(ASTLabel& ast) {} + void operator()([[maybe_unused]] const ASTLabel& ast) {} - void operator()(ASTGoto& ast) {} + void operator()([[maybe_unused]] const ASTGoto& ast) {} - void operator()(ASTDoWhile& ast) { + void operator()(const ASTDoWhile& ast) { ASTNode current = ast.nodes.GetFirst(); while (current) { Visit(current); @@ -740,11 +736,11 @@ public: } } - void operator()(ASTReturn& ast) {} + void operator()([[maybe_unused]] const ASTReturn& ast) {} - void operator()(ASTBreak& ast) {} + void operator()([[maybe_unused]] const ASTBreak& ast) {} - void Visit(ASTNode& node) { + void Visit(const ASTNode& node) { std::visit(*this, *node->GetInnerData()); node->Clear(); } diff --git a/src/video_core/shader/ast.h b/src/video_core/shader/ast.h index ba234138e..39f500284 100644 --- a/src/video_core/shader/ast.h +++ b/src/video_core/shader/ast.h @@ -48,11 +48,11 @@ public: void Init(ASTNode first, ASTNode parent); - ASTNode GetFirst() { + ASTNode GetFirst() const { return first; } - ASTNode GetLast() { + ASTNode GetLast() const { return last; } @@ -177,6 +177,10 @@ public: return &data; } + const ASTData* GetInnerData() const { + return &data; + } + ASTNode GetNext() const { return next; } @@ -189,6 +193,10 @@ public: return *manager; } + const ASTZipper& GetManager() const { + return *manager; + } + std::optional GetGotoLabel() const { auto inner = std::get_if(&data); if (inner) { From 8eb1398f8d90fb2813f438b9fffac716b6ec51d2 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 5 Oct 2019 08:17:32 -0400 Subject: [PATCH 02/11] video_core/{ast, expr}: Use std::move where applicable Avoids unnecessary atomic reference count increments and decrements. --- src/video_core/shader/ast.cpp | 20 +++++++++++--------- src/video_core/shader/ast.h | 29 +++++++++++++++-------------- src/video_core/shader/expr.cpp | 31 +++++++++++++++---------------- src/video_core/shader/expr.h | 12 ++++++------ 4 files changed, 47 insertions(+), 45 deletions(-) diff --git a/src/video_core/shader/ast.cpp b/src/video_core/shader/ast.cpp index 87c722682..986e4cd64 100644 --- a/src/video_core/shader/ast.cpp +++ b/src/video_core/shader/ast.cpp @@ -17,6 +17,7 @@ void ASTZipper::Init(const ASTNode new_first, const ASTNode parent) { ASSERT(new_first->manager == nullptr); first = new_first; last = new_first; + ASTNode current = first; while (current) { current->manager = this; @@ -92,7 +93,7 @@ void ASTZipper::InsertBefore(const ASTNode new_node, const ASTNode at_node) { new_node->manager = this; } -void ASTZipper::DetachTail(const ASTNode node) { +void ASTZipper::DetachTail(ASTNode node) { ASSERT(node->manager == this); if (node == first) { first.reset(); @@ -103,7 +104,8 @@ void ASTZipper::DetachTail(const ASTNode node) { last = node->previous; last->next.reset(); node->previous.reset(); - ASTNode current = node; + + ASTNode current = std::move(node); while (current) { current->manager = nullptr; current->parent.reset(); @@ -413,19 +415,19 @@ void ASTManager::InsertLabel(u32 address) { void ASTManager::InsertGoto(Expr condition, u32 address) { const u32 index = labels_map[address]; - const ASTNode goto_node = ASTBase::Make(main_node, condition, index); + const ASTNode goto_node = ASTBase::Make(main_node, std::move(condition), index); gotos.push_back(goto_node); program->nodes.PushBack(goto_node); } void ASTManager::InsertBlock(u32 start_address, u32 end_address) { - const ASTNode block = ASTBase::Make(main_node, start_address, end_address); - program->nodes.PushBack(block); + ASTNode block = ASTBase::Make(main_node, start_address, end_address); + program->nodes.PushBack(std::move(block)); } void ASTManager::InsertReturn(Expr condition, bool kills) { - const ASTNode node = ASTBase::Make(main_node, condition, kills); - program->nodes.PushBack(node); + ASTNode node = ASTBase::Make(main_node, std::move(condition), kills); + program->nodes.PushBack(std::move(node)); } // The decompile algorithm is based on @@ -539,11 +541,11 @@ bool ASTManager::IsBackwardsJump(ASTNode goto_node, ASTNode label_node) const { return false; } -bool ASTManager::IndirectlyRelated(ASTNode first, ASTNode second) { +bool ASTManager::IndirectlyRelated(const ASTNode& first, const ASTNode& second) const { return !(first->GetParent() == second->GetParent() || DirectlyRelated(first, second)); } -bool ASTManager::DirectlyRelated(ASTNode first, ASTNode second) { +bool ASTManager::DirectlyRelated(const ASTNode& first, const ASTNode& second) const { if (first->GetParent() == second->GetParent()) { return false; } diff --git a/src/video_core/shader/ast.h b/src/video_core/shader/ast.h index 39f500284..aad35c12e 100644 --- a/src/video_core/shader/ast.h +++ b/src/video_core/shader/ast.h @@ -71,20 +71,18 @@ public: class ASTProgram { public: - explicit ASTProgram() = default; ASTZipper nodes{}; }; class ASTIfThen { public: - explicit ASTIfThen(Expr condition) : condition(condition) {} + explicit ASTIfThen(Expr condition) : condition{std::move(condition)} {} Expr condition; ASTZipper nodes{}; }; class ASTIfElse { public: - explicit ASTIfElse() = default; ASTZipper nodes{}; }; @@ -103,7 +101,7 @@ public: class ASTVarSet { public: - explicit ASTVarSet(u32 index, Expr condition) : index{index}, condition{condition} {} + explicit ASTVarSet(u32 index, Expr condition) : index{index}, condition{std::move(condition)} {} u32 index; Expr condition; }; @@ -117,42 +115,45 @@ public: class ASTGoto { public: - explicit ASTGoto(Expr condition, u32 label) : condition{condition}, label{label} {} + explicit ASTGoto(Expr condition, u32 label) : condition{std::move(condition)}, label{label} {} Expr condition; u32 label; }; class ASTDoWhile { public: - explicit ASTDoWhile(Expr condition) : condition(condition) {} + explicit ASTDoWhile(Expr condition) : condition{std::move(condition)} {} Expr condition; ASTZipper nodes{}; }; class ASTReturn { public: - explicit ASTReturn(Expr condition, bool kills) : condition{condition}, kills{kills} {} + explicit ASTReturn(Expr condition, bool kills) + : condition{std::move(condition)}, kills{kills} {} Expr condition; bool kills; }; class ASTBreak { public: - explicit ASTBreak(Expr condition) : condition{condition} {} + explicit ASTBreak(Expr condition) : condition{std::move(condition)} {} Expr condition; }; class ASTBase { public: - explicit ASTBase(ASTNode parent, ASTData data) : parent{parent}, data{data} {} + explicit ASTBase(ASTNode parent, ASTData data) + : data{std::move(data)}, parent{std::move(parent)} {} template static ASTNode Make(ASTNode parent, Args&&... args) { - return std::make_shared(parent, ASTData(U(std::forward(args)...))); + return std::make_shared(std::move(parent), + ASTData(U(std::forward(args)...))); } void SetParent(ASTNode new_parent) { - parent = new_parent; + parent = std::move(new_parent); } ASTNode& GetParent() { @@ -247,7 +248,7 @@ public: void SetGotoCondition(Expr new_condition) { auto inner = std::get_if(&data); if (inner) { - inner->condition = new_condition; + inner->condition = std::move(new_condition); } } @@ -370,9 +371,9 @@ public: private: bool IsBackwardsJump(ASTNode goto_node, ASTNode label_node) const; - bool IndirectlyRelated(ASTNode first, ASTNode second); + bool IndirectlyRelated(const ASTNode& first, const ASTNode& second) const; - bool DirectlyRelated(ASTNode first, ASTNode second); + bool DirectlyRelated(const ASTNode& first, const ASTNode& second) const; void EncloseDoWhile(ASTNode goto_node, ASTNode label); diff --git a/src/video_core/shader/expr.cpp b/src/video_core/shader/expr.cpp index ca633ffb1..39df7a927 100644 --- a/src/video_core/shader/expr.cpp +++ b/src/video_core/shader/expr.cpp @@ -2,14 +2,21 @@ // Licensed under GPLv2 or any later version // Refer to the license.txt file included. -#pragma once - #include #include #include "video_core/shader/expr.h" namespace VideoCommon::Shader { +namespace { +bool ExprIsBoolean(const Expr& expr) { + return std::holds_alternative(*expr); +} + +bool ExprBooleanGet(const Expr& expr) { + return std::get_if(expr.get())->value; +} +} // Anonymous namespace bool ExprAnd::operator==(const ExprAnd& b) const { return (*operand1 == *b.operand1) && (*operand2 == *b.operand2); @@ -23,19 +30,11 @@ bool ExprNot::operator==(const ExprNot& b) const { return (*operand1 == *b.operand1); } -bool ExprIsBoolean(Expr expr) { - return std::holds_alternative(*expr); -} - -bool ExprBooleanGet(Expr expr) { - return std::get_if(expr.get())->value; -} - Expr MakeExprNot(Expr first) { if (std::holds_alternative(*first)) { return std::get_if(first.get())->operand1; } - return MakeExpr(first); + return MakeExpr(std::move(first)); } Expr MakeExprAnd(Expr first, Expr second) { @@ -45,7 +44,7 @@ Expr MakeExprAnd(Expr first, Expr second) { if (ExprIsBoolean(second)) { return ExprBooleanGet(second) ? first : second; } - return MakeExpr(first, second); + return MakeExpr(std::move(first), std::move(second)); } Expr MakeExprOr(Expr first, Expr second) { @@ -55,14 +54,14 @@ Expr MakeExprOr(Expr first, Expr second) { if (ExprIsBoolean(second)) { return ExprBooleanGet(second) ? second : first; } - return MakeExpr(first, second); + return MakeExpr(std::move(first), std::move(second)); } -bool ExprAreEqual(Expr first, Expr second) { +bool ExprAreEqual(const Expr& first, const Expr& second) { return (*first) == (*second); } -bool ExprAreOpposite(Expr first, Expr second) { +bool ExprAreOpposite(const Expr& first, const Expr& second) { if (std::holds_alternative(*first)) { return ExprAreEqual(std::get_if(first.get())->operand1, second); } @@ -72,7 +71,7 @@ bool ExprAreOpposite(Expr first, Expr second) { return false; } -bool ExprIsTrue(Expr first) { +bool ExprIsTrue(const Expr& first) { if (ExprIsBoolean(first)) { return ExprBooleanGet(first); } diff --git a/src/video_core/shader/expr.h b/src/video_core/shader/expr.h index 4c399cef9..1f1638520 100644 --- a/src/video_core/shader/expr.h +++ b/src/video_core/shader/expr.h @@ -28,7 +28,7 @@ using Expr = std::shared_ptr; class ExprAnd final { public: - explicit ExprAnd(Expr a, Expr b) : operand1{a}, operand2{b} {} + explicit ExprAnd(Expr a, Expr b) : operand1{std::move(a)}, operand2{std::move(b)} {} bool operator==(const ExprAnd& b) const; @@ -38,7 +38,7 @@ public: class ExprOr final { public: - explicit ExprOr(Expr a, Expr b) : operand1{a}, operand2{b} {} + explicit ExprOr(Expr a, Expr b) : operand1{std::move(a)}, operand2{std::move(b)} {} bool operator==(const ExprOr& b) const; @@ -48,7 +48,7 @@ public: class ExprNot final { public: - explicit ExprNot(Expr a) : operand1{a} {} + explicit ExprNot(Expr a) : operand1{std::move(a)} {} bool operator==(const ExprNot& b) const; @@ -105,9 +105,9 @@ Expr MakeExpr(Args&&... args) { return std::make_shared(T(std::forward(args)...)); } -bool ExprAreEqual(Expr first, Expr second); +bool ExprAreEqual(const Expr& first, const Expr& second); -bool ExprAreOpposite(Expr first, Expr second); +bool ExprAreOpposite(const Expr& first, const Expr& second); Expr MakeExprNot(Expr first); @@ -115,6 +115,6 @@ Expr MakeExprAnd(Expr first, Expr second); Expr MakeExprOr(Expr first, Expr second); -bool ExprIsTrue(Expr first); +bool ExprIsTrue(const Expr& first); } // namespace VideoCommon::Shader From 50ad74558566af897db6d7a999101e40ee544108 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 5 Oct 2019 08:37:39 -0400 Subject: [PATCH 03/11] video_core/expr: Supply operator!= along with operator== Provides logical symmetry to the interface. --- src/video_core/shader/expr.cpp | 14 +++++++++++++- src/video_core/shader/expr.h | 19 +++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/src/video_core/shader/expr.cpp b/src/video_core/shader/expr.cpp index 39df7a927..2647865d4 100644 --- a/src/video_core/shader/expr.cpp +++ b/src/video_core/shader/expr.cpp @@ -22,12 +22,24 @@ bool ExprAnd::operator==(const ExprAnd& b) const { return (*operand1 == *b.operand1) && (*operand2 == *b.operand2); } +bool ExprAnd::operator!=(const ExprAnd& b) const { + return !operator==(b); +} + bool ExprOr::operator==(const ExprOr& b) const { return (*operand1 == *b.operand1) && (*operand2 == *b.operand2); } +bool ExprOr::operator!=(const ExprOr& b) const { + return !operator==(b); +} + bool ExprNot::operator==(const ExprNot& b) const { - return (*operand1 == *b.operand1); + return *operand1 == *b.operand1; +} + +bool ExprNot::operator!=(const ExprNot& b) const { + return !operator==(b); } Expr MakeExprNot(Expr first) { diff --git a/src/video_core/shader/expr.h b/src/video_core/shader/expr.h index 1f1638520..45695c0ed 100644 --- a/src/video_core/shader/expr.h +++ b/src/video_core/shader/expr.h @@ -31,6 +31,7 @@ public: explicit ExprAnd(Expr a, Expr b) : operand1{std::move(a)}, operand2{std::move(b)} {} bool operator==(const ExprAnd& b) const; + bool operator!=(const ExprAnd& b) const; Expr operand1; Expr operand2; @@ -41,6 +42,7 @@ public: explicit ExprOr(Expr a, Expr b) : operand1{std::move(a)}, operand2{std::move(b)} {} bool operator==(const ExprOr& b) const; + bool operator!=(const ExprOr& b) const; Expr operand1; Expr operand2; @@ -51,6 +53,7 @@ public: explicit ExprNot(Expr a) : operand1{std::move(a)} {} bool operator==(const ExprNot& b) const; + bool operator!=(const ExprNot& b) const; Expr operand1; }; @@ -63,6 +66,10 @@ public: return var_index == b.var_index; } + bool operator!=(const ExprVar& b) const { + return !operator==(b); + } + u32 var_index; }; @@ -74,6 +81,10 @@ public: return predicate == b.predicate; } + bool operator!=(const ExprPredicate& b) const { + return !operator==(b); + } + u32 predicate; }; @@ -85,6 +96,10 @@ public: return cc == b.cc; } + bool operator!=(const ExprCondCode& b) const { + return !operator==(b); + } + ConditionCode cc; }; @@ -96,6 +111,10 @@ public: return value == b.value; } + bool operator!=(const ExprBoolean& b) const { + return !operator==(b); + } + bool value; }; From 43503a69bf730125b380601a919e81ca09afeb74 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 5 Oct 2019 08:40:24 -0400 Subject: [PATCH 04/11] video_core/{ast, expr}: Organize forward declaration Keeps them alphabetically sorted for readability. --- src/video_core/shader/ast.h | 18 +++++++++--------- src/video_core/shader/expr.h | 10 +++++----- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/video_core/shader/ast.h b/src/video_core/shader/ast.h index aad35c12e..6d2dc0895 100644 --- a/src/video_core/shader/ast.h +++ b/src/video_core/shader/ast.h @@ -18,17 +18,17 @@ namespace VideoCommon::Shader { class ASTBase; -class ASTProgram; -class ASTIfThen; -class ASTIfElse; -class ASTBlockEncoded; class ASTBlockDecoded; -class ASTVarSet; -class ASTGoto; -class ASTLabel; -class ASTDoWhile; -class ASTReturn; +class ASTBlockEncoded; class ASTBreak; +class ASTDoWhile; +class ASTGoto; +class ASTIfElse; +class ASTIfThen; +class ASTLabel; +class ASTProgram; +class ASTReturn; +class ASTVarSet; using ASTData = std::variant; diff --git a/src/video_core/shader/expr.h b/src/video_core/shader/expr.h index 45695c0ed..d3dcd00ec 100644 --- a/src/video_core/shader/expr.h +++ b/src/video_core/shader/expr.h @@ -15,12 +15,12 @@ using Tegra::Shader::ConditionCode; using Tegra::Shader::Pred; class ExprAnd; -class ExprOr; -class ExprNot; -class ExprPredicate; -class ExprCondCode; -class ExprVar; class ExprBoolean; +class ExprCondCode; +class ExprNot; +class ExprOr; +class ExprPredicate; +class ExprVar; using ExprData = std::variant; From 3a20d9734fa8996434895bc3d27e4b255ae98bea Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 5 Oct 2019 08:43:44 -0400 Subject: [PATCH 05/11] video_core/ast: Default the move constructor and assignment operator This is behaviorally equivalent and also fixes a bug where some members weren't being moved over. --- src/video_core/shader/ast.cpp | 24 ------------------------ src/video_core/shader/ast.h | 4 ++-- 2 files changed, 2 insertions(+), 26 deletions(-) diff --git a/src/video_core/shader/ast.cpp b/src/video_core/shader/ast.cpp index 986e4cd64..2627c563c 100644 --- a/src/video_core/shader/ast.cpp +++ b/src/video_core/shader/ast.cpp @@ -374,30 +374,6 @@ void ASTManager::Init() { false_condition = MakeExpr(false); } -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}, - disable_else_derivation{other.disable_else_derivation} { - other.main_node.reset(); -} - -ASTManager& ASTManager::operator=(ASTManager&& other) noexcept { - full_decompile = other.full_decompile; - 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; - disable_else_derivation = other.disable_else_derivation; - - other.main_node.reset(); - return *this; -} - void ASTManager::DeclareLabel(u32 address) { const auto pair = labels_map.emplace(address, labels_count); if (pair.second) { diff --git a/src/video_core/shader/ast.h b/src/video_core/shader/ast.h index 6d2dc0895..d280ed143 100644 --- a/src/video_core/shader/ast.h +++ b/src/video_core/shader/ast.h @@ -313,8 +313,8 @@ public: ASTManager(const ASTManager& o) = delete; ASTManager& operator=(const ASTManager& other) = delete; - ASTManager(ASTManager&& other) noexcept; - ASTManager& operator=(ASTManager&& other) noexcept; + ASTManager(ASTManager&& other) noexcept = default; + ASTManager& operator=(ASTManager&& other) noexcept = default; void Init(); From 5a0a9c7449f4f264fae8619d3881c6c0e09865ef Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 5 Oct 2019 08:45:28 -0400 Subject: [PATCH 06/11] video_core/ast: Replace std::string with a constexpr std::string_view Same behavior, but without the need to heap allocate --- src/video_core/shader/ast.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/video_core/shader/ast.cpp b/src/video_core/shader/ast.cpp index 2627c563c..f2ab0cc00 100644 --- a/src/video_core/shader/ast.cpp +++ b/src/video_core/shader/ast.cpp @@ -350,11 +350,9 @@ private: std::string tabs_memo{}; u32 memo_scope{}; - static std::string tabs; + static constexpr std::string_view tabs{" "}; }; -std::string ASTPrinter::tabs = " "; - std::string ASTManager::Print() { ASTPrinter printer{}; printer.Visit(main_node); From 3c54edae2438bd0dced6c552b42ff2be4eebd6b6 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 5 Oct 2019 08:46:54 -0400 Subject: [PATCH 07/11] video_core/ast: Eliminate variable shadowing warnings --- src/video_core/shader/ast.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/video_core/shader/ast.cpp b/src/video_core/shader/ast.cpp index f2ab0cc00..6eba78025 100644 --- a/src/video_core/shader/ast.cpp +++ b/src/video_core/shader/ast.cpp @@ -468,10 +468,10 @@ void ASTManager::Decompile() { } labels.clear(); } else { - auto it = labels.begin(); - while (it != labels.end()) { + auto label_it = labels.begin(); + while (label_it != labels.end()) { bool can_remove = true; - ASTNode label = *it; + ASTNode label = *label_it; for (const ASTNode& goto_node : gotos) { const auto label_index = goto_node->GetGotoLabel(); if (!label_index) { From 6c41d1cd7eadf1030c02d661d7f360b98f4a8943 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 5 Oct 2019 08:48:15 -0400 Subject: [PATCH 08/11] video_core/ast: Make ShowCurrentState() take a string_view instead of std::string Allows the function to be non-allocating in terms of the output string. --- src/video_core/shader/ast.cpp | 2 +- src/video_core/shader/ast.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/video_core/shader/ast.cpp b/src/video_core/shader/ast.cpp index 6eba78025..436d45f4b 100644 --- a/src/video_core/shader/ast.cpp +++ b/src/video_core/shader/ast.cpp @@ -549,7 +549,7 @@ bool ASTManager::DirectlyRelated(const ASTNode& first, const ASTNode& second) co return min->GetParent() == max->GetParent(); } -void ASTManager::ShowCurrentState(std::string state) { +void ASTManager::ShowCurrentState(std::string_view state) { LOG_CRITICAL(HW_GPU, "\nState {}:\n\n{}\n", state, Print()); SanityCheck(); } diff --git a/src/video_core/shader/ast.h b/src/video_core/shader/ast.h index d280ed143..5a77c60cb 100644 --- a/src/video_core/shader/ast.h +++ b/src/video_core/shader/ast.h @@ -332,7 +332,7 @@ public: void Decompile(); - void ShowCurrentState(std::string state); + void ShowCurrentState(std::string_view state); void SanityCheck(); From d82b181d445441ce84612c38d748d3d5a6f8854c Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 5 Oct 2019 08:50:00 -0400 Subject: [PATCH 09/11] video_core/ast: Unindent most of IsFullyDecompiled() by one level --- src/video_core/shader/ast.h | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/video_core/shader/ast.h b/src/video_core/shader/ast.h index 5a77c60cb..d7bf11821 100644 --- a/src/video_core/shader/ast.h +++ b/src/video_core/shader/ast.h @@ -340,20 +340,20 @@ public: bool IsFullyDecompiled() const { if (full_decompile) { - return gotos.size() == 0; - } else { - for (ASTNode goto_node : gotos) { - auto label_index = goto_node->GetGotoLabel(); - if (!label_index) { - return false; - } - ASTNode glabel = labels[*label_index]; - if (IsBackwardsJump(goto_node, glabel)) { - return false; - } - } - return true; + return gotos.empty(); } + + for (ASTNode goto_node : gotos) { + auto label_index = goto_node->GetGotoLabel(); + if (!label_index) { + return false; + } + ASTNode glabel = labels[*label_index]; + if (IsBackwardsJump(goto_node, glabel)) { + return false; + } + } + return true; } ASTNode GetProgram() const { From 25702b6256e33c3c2383a8674ecd69b5e7425509 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 5 Oct 2019 08:52:20 -0400 Subject: [PATCH 10/11] video_core/control_flow: Eliminate pessimizing moves These can inhibit the ability of a compiler to perform RVO. --- src/video_core/shader/control_flow.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/video_core/shader/control_flow.cpp b/src/video_core/shader/control_flow.cpp index 3c3a41ba6..70e1d3239 100644 --- a/src/video_core/shader/control_flow.cpp +++ b/src/video_core/shader/control_flow.cpp @@ -479,7 +479,7 @@ std::unique_ptr ScanFlow(const ProgramCode& program_code, auto result_out = std::make_unique(); if (settings.depth == CompileDepth::BruteForce) { result_out->settings.depth = CompileDepth::BruteForce; - return std::move(result_out); + return result_out; } CFGRebuildState state{program_code, program_size, start_address}; @@ -490,7 +490,7 @@ std::unique_ptr ScanFlow(const ProgramCode& program_code, while (!state.inspect_queries.empty()) { if (!TryInspectAddress(state)) { result_out->settings.depth = CompileDepth::BruteForce; - return std::move(result_out); + return result_out; } } @@ -535,9 +535,10 @@ std::unique_ptr ScanFlow(const ProgramCode& program_code, result_out->settings.depth = settings.depth; result_out->manager = std::move(manager); result_out->end = state.block_info.back().end + 1; - return std::move(result_out); + return result_out; } } + result_out->start = start_address; result_out->settings.depth = use_flow_stack ? CompileDepth::FlowStack : CompileDepth::NoFlowStack; @@ -557,8 +558,9 @@ std::unique_ptr ScanFlow(const ProgramCode& program_code, } if (!use_flow_stack) { result_out->labels = std::move(state.labels); - return std::move(result_out); + return result_out; } + auto back = result_out->blocks.begin(); auto next = std::next(back); while (next != result_out->blocks.end()) { @@ -570,6 +572,7 @@ std::unique_ptr ScanFlow(const ProgramCode& program_code, back = next; ++next; } - return std::move(result_out); + + return result_out; } } // namespace VideoCommon::Shader From f883cd4f0ebf15916610fc5f1f006f81830eabbc Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 5 Oct 2019 08:55:08 -0400 Subject: [PATCH 11/11] video_core/control_flow: Eliminate variable shadowing warnings --- src/video_core/shader/control_flow.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/video_core/shader/control_flow.cpp b/src/video_core/shader/control_flow.cpp index 70e1d3239..268d1aed0 100644 --- a/src/video_core/shader/control_flow.cpp +++ b/src/video_core/shader/control_flow.cpp @@ -530,12 +530,12 @@ std::unique_ptr ScanFlow(const ProgramCode& program_code, state.manager->ShowCurrentState("Of Shader"); state.manager->Clear(); } else { - auto result_out = std::make_unique(); - result_out->start = start_address; - result_out->settings.depth = settings.depth; - result_out->manager = std::move(manager); - result_out->end = state.block_info.back().end + 1; - return result_out; + auto characteristics = std::make_unique(); + characteristics->start = start_address; + characteristics->settings.depth = settings.depth; + characteristics->manager = std::move(manager); + characteristics->end = state.block_info.back().end + 1; + return characteristics; } }