[Mesa-dev] [PATCH] r600g/sb: fix issues cause by GLSL switching to loops for switch

[Glenn Kennard]

On Fri, 28 Nov 2014 04:36:42 +0100, Dave Airlie <airlied at gmail.com> wrote:

> From: Dave Airlie <airlied at redhat.com>
>
> Since 73dd50acf6d244979c2a657906aa56d3ac60d550
> glsl: implement switch flow control using a loop
>
> The SB backend was falling over in an assert or crashing.
>
> Tracked this down to the loops having no repeats, but requiring
> a working break, initial code just called the loop handler for
> all non-if statements, but this caused a regression in
> tests/shaders/dead-code-break-interaction.shader_test.
> So I had to add further code to detect if all the departure
> nodes are empty and avoid generating an empty loop for that case.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86089
> Signed-off-by: Dave Airlie <airlied at redhat.com>
> ---
>  src/gallium/drivers/r600/sb/sb_bc_finalize.cpp | 51  
> ++++++++++++++++++--------
>  1 file changed, 36 insertions(+), 15 deletions(-)
>
> diff --git a/src/gallium/drivers/r600/sb/sb_bc_finalize.cpp  
> b/src/gallium/drivers/r600/sb/sb_bc_finalize.cpp
> index f0849ca..d91ffa5 100644
> --- a/src/gallium/drivers/r600/sb/sb_bc_finalize.cpp
> +++ b/src/gallium/drivers/r600/sb/sb_bc_finalize.cpp
> @@ -46,15 +46,22 @@ int bc_finalizer::run() {
>  	for (regions_vec::reverse_iterator I = rv.rbegin(), E = rv.rend(); I  
> != E;
>  			++I) {
>  		region_node *r = *I;
> -
> +		bool is_if = false;
>  		assert(r);
> -		bool loop = r->is_loop();
> +		assert(r->first);
> +		if (r->first->is_container()) {
> +			container_node *repdep1 = static_cast<container_node*>(r->first);
> +			assert(repdep1->is_depart() || repdep1->is_repeat());
> +			if_node *n_if = static_cast<if_node*>(repdep1->first);
> +			if (n_if && n_if->is_if())
> +				is_if = true;
> +		}
> -		if (loop)
> -			finalize_loop(r);
> -		else
> +		if (is_if)
>  			finalize_if(r);
> +		else
> +			finalize_loop(r);
> 		r->expand();
>  	}
> @@ -112,16 +119,31 @@ void bc_finalizer::finalize_loop(region_node* r) {
> 	cf_node *loop_start = sh.create_cf(CF_OP_LOOP_START_DX10);
>  	cf_node *loop_end = sh.create_cf(CF_OP_LOOP_END);
> +	bool has_instr = false;
> +
> +	if (!r->is_loop()) {
> +		for (depart_vec::iterator I = r->departs.begin(), E =  
> r->departs.end();
> +		     I != E; ++I) {
> +			depart_node *dep = *I;
> +			if (!dep->empty())
> +				has_instr = true;

could break here

> +		}
> +	} else
> +		has_instr = true;
> -	loop_start->jump_after(loop_end);
> -	loop_end->jump_after(loop_start);
> +	if (has_instr) {
> +		loop_start->jump_after(loop_end);
> +		loop_end->jump_after(loop_start);
> +	}
> 	for (depart_vec::iterator I = r->departs.begin(), E = r->departs.end();
>  			I != E; ++I) {
>  		depart_node *dep = *I;
> -		cf_node *loop_break = sh.create_cf(CF_OP_LOOP_BREAK);
> -		loop_break->jump(loop_end);
> -		dep->push_back(loop_break);
> +		if (has_instr) {
> +			cf_node *loop_break = sh.create_cf(CF_OP_LOOP_BREAK);
> +			loop_break->jump(loop_end);
> +			dep->push_back(loop_break);
> +		}
>  		dep->expand();
>  	}
> @@ -137,8 +159,10 @@ void bc_finalizer::finalize_loop(region_node* r) {
>  		rep->expand();
>  	}
> -	r->push_front(loop_start);
> -	r->push_back(loop_end);
> +	if (has_instr) {
> +		r->push_front(loop_start);
> +		r->push_back(loop_end);
> +	}
>  }
> void bc_finalizer::finalize_if(region_node* r) {
> @@ -168,9 +192,6 @@ void bc_finalizer::finalize_if(region_node* r) {
> 	if (n_if) {
> -
> -		assert(n_if->is_if());

shouldn't need to remove this assertion

> -
>  		container_node *repdep2 = static_cast<container_node*>(n_if->first);
>  		assert(repdep2->is_depart() || repdep2->is_repeat());
>

I think i've managed to convince myself the above logic is correct, so
Reviewed-By: Glenn Kennard <glenn.kennard at gmail.com>