[Mesa-dev] [PATCH] r600g/sb: Fix memory leak by reworking uses list (rebased)

Constantine Charlamov Hi-Angel at yandex.ru
Thu Feb 23 21:00:07 UTC 2017


The author is Heiko Przybyl(CC'ing), the patch is rebased on top of Bartosz Tomczyk's one per Dieter N├╝tzel's comment.
Tested-by: Constantine Charlamov <Hi-Angel at yandex.ru>

--------------
When fixing the stalls on evergreen I introduced leaking of the useinfo
structure(s). Sorry. Instead of allocating a new object to hold 3 values
where only one is actually used, rework the list to just store the node
pointer. Thus no allocating and deallocation is needed. Since use_info
and use_kind aren't used anywhere, drop them and reduce code complexity.
This might also save some small amount of cycles.

Thanks to Bartosz Tomczyk for finding the bug.

Reported-by: Bartosz Tomczyk <bartosz.tomczyk86 at gmail.com <https://lists.freedesktop.org/mailman/listinfo/mesa-dev>>
Signed-off-by: Heiko Przybyl <lil_tux at web.de <https://lists.freedesktop.org/mailman/listinfo/mesa-dev>>
Supersedes: https://patchwork.freedesktop.org/patch/135852

---
 src/gallium/drivers/r600/sb/sb_def_use.cpp  | 29 +++++++++++------------------
 src/gallium/drivers/r600/sb/sb_gcm.cpp      | 16 ++++++++--------
 src/gallium/drivers/r600/sb/sb_ir.h         | 23 ++---------------------
 src/gallium/drivers/r600/sb/sb_valtable.cpp | 21 +++++++--------------
 4 files changed, 28 insertions(+), 61 deletions(-)

diff --git a/src/gallium/drivers/r600/sb/sb_def_use.cpp b/src/gallium/drivers/r600/sb/sb_def_use.cpp
index a512d92086..68ab4ca26c 100644
--- a/src/gallium/drivers/r600/sb/sb_def_use.cpp
+++ b/src/gallium/drivers/r600/sb/sb_def_use.cpp
@@ -106,58 +106,51 @@ void def_use::process_defs(node *n, vvec &vv, bool arr_def) {
 }
 
 void def_use::process_uses(node* n) {
-    unsigned k = 0;
-
-    for (vvec::iterator I = n->src.begin(), E = n->src.end(); I != E;
-            ++I, ++k) {
+    for (vvec::iterator I = n->src.begin(), E = n->src.end(); I != E; ++I) {
         value *v = *I;
         if (!v || v->is_readonly())
             continue;
 
         if (v->is_rel()) {
             if (!v->rel->is_readonly())
-                v->rel->add_use(n, UK_SRC_REL, k);
+                v->rel->add_use(n);
 
-            unsigned k2 = 0;
             for (vvec::iterator I = v->muse.begin(), E = v->muse.end();
-                    I != E; ++I, ++k2) {
+                    I != E; ++I) {
                 value *v = *I;
                 if (!v)
                     continue;
 
-                v->add_use(n, UK_MAYUSE, k2);
+                v->add_use(n);
             }
         } else
-            v->add_use(n, UK_SRC, k);
+            v->add_use(n);
     }
 
-    k = 0;
-    for (vvec::iterator I = n->dst.begin(), E = n->dst.end(); I != E;
-            ++I, ++k) {
+    for (vvec::iterator I = n->dst.begin(), E = n->dst.end(); I != E; ++I) {
         value *v = *I;
         if (!v || !v->is_rel())
             continue;
 
         if (!v->rel->is_readonly())
-            v->rel->add_use(n, UK_DST_REL, k);
-        unsigned k2 = 0;
+            v->rel->add_use(n);
         for (vvec::iterator I = v->muse.begin(), E = v->muse.end();
-                I != E; ++I, ++k2) {
+                I != E; ++I) {
             value *v = *I;
             if (!v)
                 continue;
 
-            v->add_use(n, UK_MAYDEF, k2);
+            v->add_use(n);
         }
     }
 
     if (n->pred)
-        n->pred->add_use(n, UK_PRED, 0);
+        n->pred->add_use(n);
 
     if (n->type == NT_IF) {
         if_node *i = static_cast<if_node*>(n);
         if (i->cond)
-            i->cond->add_use(i, UK_COND, 0);
+            i->cond->add_use(i);
     }
 }
 
diff --git a/src/gallium/drivers/r600/sb/sb_gcm.cpp b/src/gallium/drivers/r600/sb/sb_gcm.cpp
index 9c75389ada..7b43a32818 100644
--- a/src/gallium/drivers/r600/sb/sb_gcm.cpp
+++ b/src/gallium/drivers/r600/sb/sb_gcm.cpp
@@ -200,27 +200,27 @@ void gcm::td_release_val(value *v) {
     );
 
     for (uselist::iterator I = v->uses.begin(), E = v->uses.end(); I != E; ++I) {
-        use_info *u = *I;
-        if (u->op->parent != &pending) {
+        node *op = *I;
+        if (op->parent != &pending) {
             continue;
         }
 
         GCM_DUMP(
             sblog << "td    used in ";
-            dump::dump_op(u->op);
+            dump::dump_op(op);
             sblog << "\n";
         );
 
-        assert(uses[u->op] > 0);
-        if (--uses[u->op] == 0) {
+        assert(uses[op] > 0);
+        if (--uses[op] == 0) {
             GCM_DUMP(
                 sblog << "td        released : ";
-                dump::dump_op(u->op);
+                dump::dump_op(op);
                 sblog << "\n";
             );
 
-            pending.remove_node(u->op);
-            ready.push_back(u->op);
+            pending.remove_node(op);
+            ready.push_back(op);
         }
     }
 
diff --git a/src/gallium/drivers/r600/sb/sb_ir.h b/src/gallium/drivers/r600/sb/sb_ir.h
index 74c0549a81..ec973e7bfc 100644
--- a/src/gallium/drivers/r600/sb/sb_ir.h
+++ b/src/gallium/drivers/r600/sb/sb_ir.h
@@ -435,26 +435,7 @@ sb_ostream& operator << (sb_ostream &o, value &v);
 
 typedef uint32_t value_hash;
 
-enum use_kind {
-    UK_SRC,
-    UK_SRC_REL,
-    UK_DST_REL,
-    UK_MAYDEF,
-    UK_MAYUSE,
-    UK_PRED,
-    UK_COND
-};
-
-struct use_info {
-    node *op;
-    use_kind kind;
-    int arg;
-
-    use_info(node *n, use_kind kind, int arg)
-        : op(n), kind(kind), arg(arg) {}
-};
-
-typedef std::list< use_info * > uselist;
+typedef std::list< node * > uselist;
 
 enum constraint_kind {
     CK_SAME_REG,
@@ -585,7 +566,7 @@ public:
                 && literal_value != literal(1.0);
     }
 
-    void add_use(node *n, use_kind kind, int arg);
+    void add_use(node *n);
     void remove_use(const node *n);
 
     value_hash hash();
diff --git a/src/gallium/drivers/r600/sb/sb_valtable.cpp b/src/gallium/drivers/r600/sb/sb_valtable.cpp
index d31a1b76d5..a85537c2ad 100644
--- a/src/gallium/drivers/r600/sb/sb_valtable.cpp
+++ b/src/gallium/drivers/r600/sb/sb_valtable.cpp
@@ -212,21 +212,20 @@ void value_table::get_values(vvec& v) {
     }
 }
 
-void value::add_use(node* n, use_kind kind, int arg) {
+void value::add_use(node* n) {
     if (0) {
     sblog << "add_use ";
     dump::dump_val(this);
     sblog << "   =>  ";
     dump::dump_op(n);
-    sblog << "     kind " << kind << "    arg " << arg << "\n";
     }
-    uses.push_back(new use_info(n, kind, arg));
+    uses.push_back(n);
 }
 
 struct use_node_comp {
     explicit use_node_comp(const node *n) : n(n) {}
-    bool operator() (const use_info *u) {
-        return u->op->hash() == n->hash();
+    bool operator() (const node *o) {
+        return o->hash() == n->hash();
     }
 
     private:
@@ -239,9 +238,7 @@ void value::remove_use(const node *n) {
 
     if (it != uses.end())
     {
-        // TODO assert((*it)->kind == kind) ?
-        // TODO assert((*it)->arg == arg) ?
-        delete *it;
+        // We only ever had a pointer, so don't delete it here
         uses.erase(it);
     }
 }
@@ -291,12 +288,8 @@ bool value::is_prealloc() {
 }
 
 void value::delete_uses() {
-    for (uselist::iterator it = uses.begin(); it != uses.end(); ++it)
-    {
-        delete *it;
-    }
-
-    uses.clear();
+    // We only ever had pointers, so don't delete them here
+    uses.erase(uses.begin(), uses.end());
 }
 
 void ra_constraint::update_values() {
-- 
2.11.1



More information about the mesa-dev mailing list