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

Constantine Kharlamov Hi-Angel at yandex.ru
Mon Mar 20 12:23:10 UTC 2017


The patch still applies to master, rebase is unneeded.

On 20.03.2017 14:53, Dieter Nützel wrote:
> Hello Constantine,
> 
> rebase again, please?
> Shouldn't we try to land?
> 
> Thanks,
> Dieter
> 
> Am 23.02.2017 22:00, schrieb Constantine Charlamov:
>> 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() {
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


More information about the mesa-dev mailing list