[Mesa-dev] [PATCH] r600/sb: Force ELSE path if the byte code started off with it

[Roland Scheidegger]

Am 29.01.2018 um 13:15 schrieb Gert Wollny:
> sb optimized away the ELSE in a construct like
> 
>    while (foo) {
>       if (bar ) {
>          do something;
>       } else {
>          break;
>       }
>    }
> 
> resulting in
> 
>    while (foo) {
>       if (bar ) {
>          do something;
>          break;
>       }
>    }
> 
> which is obviously wrong.
> 
> With this patch an ELSE is inserted also if there was one before optimization.
> This fixes piglit shaders at ssa@fs-if-def-else-break.
> 
> It might introduce a penalty by keeping an ELSE instruction when it can
> actually be really optimized away though.

Am I correct assuming that for something like
   while (foo) {
      if (bar) {
         do something;
      } else {
         /* nothing */
      }
   }
The else clause wouldn't get optimized away neither?
(This of course is a trivial example, but I suppose it would extend to
cases where the optimizer actually optimized away the alu instructions
in the else clause).
In this case, this indeed looks rather harsh.
I would have said in theory somehow the break should be some op which
(despite not having a dst) has side-effects so it would not be subject
to elimination somewhere (as there would be a if->next node in this case
then).
Albeit I'm completely oblivious how it really gets optimized away, is
that based on liveness analysis or something? The sb code is a mystery
to me...

Roland


> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101442
> ---
> I think there should be a better way to ensure that the else is inserted 
> in this case, but so far it elluded me. 
> 
> I have no mesa-git write access. 
> 
>  src/gallium/drivers/r600/sb/sb_bc_finalize.cpp | 4 +---
>  src/gallium/drivers/r600/sb/sb_bc_parser.cpp   | 5 ++++-
>  src/gallium/drivers/r600/sb/sb_ir.h            | 3 ++-
>  3 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/src/gallium/drivers/r600/sb/sb_bc_finalize.cpp b/src/gallium/drivers/r600/sb/sb_bc_finalize.cpp
> index 099b295f18..4450247caf 100644
> --- a/src/gallium/drivers/r600/sb/sb_bc_finalize.cpp
> +++ b/src/gallium/drivers/r600/sb/sb_bc_finalize.cpp
> @@ -208,9 +208,7 @@ void bc_finalizer::finalize_if(region_node* r) {
>  		r->push_front(if_jump);
>  		r->push_back(if_pop);
>  
> -		bool has_else = n_if->next;
> -
> -		if (has_else) {
> +		if (n_if->has_else || n_if->next) {
>  			cf_node *nelse = sh.create_cf(CF_OP_ELSE);
>  			n_if->insert_after(nelse);
>  			if_jump->jump(nelse);
> diff --git a/src/gallium/drivers/r600/sb/sb_bc_parser.cpp b/src/gallium/drivers/r600/sb/sb_bc_parser.cpp
> index 970e4141d5..f37067b8e2 100644
> --- a/src/gallium/drivers/r600/sb/sb_bc_parser.cpp
> +++ b/src/gallium/drivers/r600/sb/sb_bc_parser.cpp
> @@ -959,8 +959,11 @@ int bc_parser::prepare_if(cf_node* c) {
>  
>  	c->insert_before(reg);
>  
> -	if (c_else != end)
> +	if (c_else != end) {
> +		n_if->has_else = true; 
>  		dep->move(c_else, end);
> +	}
> +
>  	dep2->move(c, end);
>  
>  	reg->push_back(dep);
> diff --git a/src/gallium/drivers/r600/sb/sb_ir.h b/src/gallium/drivers/r600/sb/sb_ir.h
> index bee947504e..af9b161597 100644
> --- a/src/gallium/drivers/r600/sb/sb_ir.h
> +++ b/src/gallium/drivers/r600/sb/sb_ir.h
> @@ -1091,12 +1091,13 @@ public:
>  
>  class if_node : public container_node {
>  protected:
> -	if_node() : container_node(NT_IF, NST_LIST), cond() {};
> +	if_node() : container_node(NT_IF, NST_LIST), cond(), has_else(false) {};
>  public:
>  	value *cond; // glued to pseudo output (dst[2]) of the PRED_SETxxx
>  
>  	virtual bool accept(vpass &p, bool enter);
>  
> +	bool has_else;
>  	friend class shader;
>  };
>  
>