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

Dieter N├╝tzel Dieter at nuetzel-hh.de
Wed Feb 8 23:43:19 UTC 2017


Am 07.02.2017 22:30, schrieb Heiko Przybyl:
> 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>
> Signed-off-by: Heiko Przybyl <lil_tux at web.de>
> 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 | 13 ++++++-------
>  4 files changed, 27 insertions(+), 54 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();

Hello Heiko,

the below part have to be rebased after Marek's commit on master.

Greetings,
Dieter

> diff --git a/src/gallium/drivers/r600/sb/sb_valtable.cpp
> b/src/gallium/drivers/r600/sb/sb_valtable.cpp
> index a8b7b49cd4..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,8 +238,7 @@ void value::remove_use(const node *n) {
> 
>  	if (it != uses.end())
>  	{
> -		// TODO assert((*it)->kind == kind) ?
> -		// TODO assert((*it)->arg == arg) ?
> +		// We only ever had a pointer, so don't delete it here
>  		uses.erase(it);
>  	}
>  }
> @@ -290,6 +288,7 @@ bool value::is_prealloc() {
>  }
> 
>  void value::delete_uses() {
> +	// We only ever had pointers, so don't delete them here
>  	uses.erase(uses.begin(), uses.end());
>  }


More information about the mesa-dev mailing list