[Mesa-dev] [PATCH] r600g/sb: Fix memory leak by reworking uses list (rebased)
Marek Olšák
maraeo at gmail.com
Mon Mar 20 16:35:32 UTC 2017
Please rebase. It doesn't apply anymore.
Marek
On Thu, Feb 23, 2017 at 10:00 PM, Constantine Charlamov
<Hi-Angel at yandex.ru> wrote:
> 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
>
> _______________________________________________
> 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