From 8367841e141e2bfcc2fa6f2101ae53e742501460 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Wed, 27 Mar 2019 17:37:47 -0700 Subject: [PATCH 1/3] IRGraphVisitor should increment refcount of visited items Currently, visiting temporarily-created items has the risk of the memory for the temporary item being re-used during the visit; if that happens, subsequent temporary items with the same memory will be skipped. --- src/IRVisitor.cpp | 20 ++++++++++++++++++++ src/IRVisitor.h | 10 ++++++++++ 2 files changed, 30 insertions(+) diff --git a/src/IRVisitor.cpp b/src/IRVisitor.cpp index 7fc3c8480bab..faeaca419c71 100644 --- a/src/IRVisitor.cpp +++ b/src/IRVisitor.cpp @@ -256,10 +256,29 @@ void IRVisitor::visit(const Shuffle *op) { } } +IRGraphVisitor::IRGraphVisitor(const IRGraphVisitor &that) : visited(that.visited) { + for (const IRNode *v : visited) { + v->ref_count.increment(); + } +} + +IRGraphVisitor::IRGraphVisitor(IRGraphVisitor &&that) { + std::swap(this->visited, that.visited); +} + +IRGraphVisitor::~IRGraphVisitor() { + for (const IRNode *v : visited) { + if (v->ref_count.decrement() == 0) { + delete v; + } + } +} + void IRGraphVisitor::include(const Expr &e) { auto r = visited.insert(e.get()); if (r.second) { // Was newly inserted + e.get()->ref_count.increment(); e.accept(this); } } @@ -268,6 +287,7 @@ void IRGraphVisitor::include(const Stmt &s) { auto r = visited.insert(s.get()); if (r.second) { // Was newly inserted + s.get()->ref_count.increment(); s.accept(this); } } diff --git a/src/IRVisitor.h b/src/IRVisitor.h index 70d7d2f43f26..e13f504accb0 100644 --- a/src/IRVisitor.h +++ b/src/IRVisitor.h @@ -80,6 +80,16 @@ class IRVisitor { * without visiting the same node twice. This is for passes that are * capable of interpreting the IR as a DAG instead of a tree. */ class IRGraphVisitor : public IRVisitor { +public: + IRGraphVisitor() = default; + virtual ~IRGraphVisitor(); + + IRGraphVisitor(IRGraphVisitor &&that); + IRGraphVisitor(const IRGraphVisitor &that); + + void operator=(const IRGraphVisitor &that) = delete; + void operator=(IRGraphVisitor &&that) = delete; + protected: /** By default these methods add the node to the visited set, and * return whether or not it was already there. If it wasn't there, From d3e242b50673ae7c62d8ee8c500638baf57ab91a Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Thu, 28 Mar 2019 10:32:08 -0700 Subject: [PATCH 2/3] Use IRHandle instead of explicit refcounting --- src/IRVisitor.cpp | 20 -------------------- src/IRVisitor.h | 12 +----------- 2 files changed, 1 insertion(+), 31 deletions(-) diff --git a/src/IRVisitor.cpp b/src/IRVisitor.cpp index faeaca419c71..7fc3c8480bab 100644 --- a/src/IRVisitor.cpp +++ b/src/IRVisitor.cpp @@ -256,29 +256,10 @@ void IRVisitor::visit(const Shuffle *op) { } } -IRGraphVisitor::IRGraphVisitor(const IRGraphVisitor &that) : visited(that.visited) { - for (const IRNode *v : visited) { - v->ref_count.increment(); - } -} - -IRGraphVisitor::IRGraphVisitor(IRGraphVisitor &&that) { - std::swap(this->visited, that.visited); -} - -IRGraphVisitor::~IRGraphVisitor() { - for (const IRNode *v : visited) { - if (v->ref_count.decrement() == 0) { - delete v; - } - } -} - void IRGraphVisitor::include(const Expr &e) { auto r = visited.insert(e.get()); if (r.second) { // Was newly inserted - e.get()->ref_count.increment(); e.accept(this); } } @@ -287,7 +268,6 @@ void IRGraphVisitor::include(const Stmt &s) { auto r = visited.insert(s.get()); if (r.second) { // Was newly inserted - s.get()->ref_count.increment(); s.accept(this); } } diff --git a/src/IRVisitor.h b/src/IRVisitor.h index e13f504accb0..d24cc609426f 100644 --- a/src/IRVisitor.h +++ b/src/IRVisitor.h @@ -80,16 +80,6 @@ class IRVisitor { * without visiting the same node twice. This is for passes that are * capable of interpreting the IR as a DAG instead of a tree. */ class IRGraphVisitor : public IRVisitor { -public: - IRGraphVisitor() = default; - virtual ~IRGraphVisitor(); - - IRGraphVisitor(IRGraphVisitor &&that); - IRGraphVisitor(const IRGraphVisitor &that); - - void operator=(const IRGraphVisitor &that) = delete; - void operator=(IRGraphVisitor &&that) = delete; - protected: /** By default these methods add the node to the visited set, and * return whether or not it was already there. If it wasn't there, @@ -102,7 +92,7 @@ class IRGraphVisitor : public IRVisitor { private: /** The nodes visited so far */ - std::set visited; + std::set visited; protected: /** These methods should call 'include' on the children to only From 652b4b3ca95d57e322a212662d2535e17f98a5a6 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Thu, 28 Mar 2019 10:34:08 -0700 Subject: [PATCH 3/3] Correct no-longer-correct comment in Codegen_C.cpp --- src/CodeGen_C.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/CodeGen_C.cpp b/src/CodeGen_C.cpp index dfa1b73564a9..a29f2a78290c 100644 --- a/src/CodeGen_C.cpp +++ b/src/CodeGen_C.cpp @@ -275,10 +275,6 @@ class TypeInfoGatherer : public IRGraphVisitor { include_type(op->type); if (op->is_intrinsic(Call::lerp)) { // lower_lerp() can synthesize wider vector types. - // It's not safe to feed temporary Exprs into IRGraphVisitor - // (it tracks the seen values by IRNode*, which could get recycled - // if we are unlucky), so just add widened versions of any - // types present -- it's safe to add types we might not use. for (auto &a : op->args) { include_lerp_types(a.type()); }