[Mesa-dev] [PATCH 7/9] glsl: Convert ir_call to be a statement rather than a value.

[Eric Anholt]

On Wed, 28 Mar 2012 20:33:06 -0700, Kenneth Graunke <kenneth at whitecape.org> wrote:
> Aside from ir_call, our IR is cleanly split into two classes:
> - Statements (typeless; used for side effects, control flow)
> - Values (deeply nestable, pure, typed expression trees)
> diff --git a/src/glsl/ir_basic_block.cpp b/src/glsl/ir_basic_block.cpp
> index a833825..5ebbf6f 100644
> --- a/src/glsl/ir_basic_block.cpp
> +++ b/src/glsl/ir_basic_block.cpp
> @@ -122,24 +97,9 @@ void call_for_basic_blocks(exec_list *instructions,
>  
>  	    call_for_basic_blocks(&ir_sig->body, callback, data);
>  	 }
> -      } else if (ir->as_assignment()) {
> -	 /* If there's a call in the expression tree being assigned,
> -	  * then that ends the BB too.
> -	  *
> -	  * The assumption is that any consumer of the basic block
> -	  * walker is fine with the fact that the call is somewhere in
> -	  * the tree even if portions of the tree may be evaluated
> -	  * after the call.
> -	  *
> -	  * A consumer that has an issue with this could not process
> -	  * the last instruction of the basic block.  If doing so,
> -	  * expression flattener may be useful before using the basic
> -	  * block finder to get more maximal basic blocks out.
> -	  */
> -	 if (ir_has_call(ir)) {
> -	    callback(leader, ir, data);
> -	    leader = NULL;
> -	 }
> +      } else if (ir->as_call()) {
> +	 callback(leader, ir, data);
> +	 leader = NULL;

There was already a block checking for ir->as_call() above, so this can
just be dropped.

> diff --git a/src/glsl/ir_validate.cpp b/src/glsl/ir_validate.cpp
> index 101d999..385ef12 100644
> --- a/src/glsl/ir_validate.cpp
> +++ b/src/glsl/ir_validate.cpp
> @@ -548,6 +548,23 @@ ir_validate::visit_enter(ir_call *ir)
>        abort();
>     }
>  
> +   if (callee->return_type == glsl_type::void_type) {
> +      if (ir->return_deref != NULL) {
> +	 printf("ir_call to void function has storage for a return value\n");
> +	 abort();
> +      }
> +   } else {
> +      if (ir->return_deref == NULL) {
> +	 printf("ir_call has non-void callee but no return storage\n");
> +	 abort();
> +      }
> +      if (callee->return_type != ir->return_deref->type) {
> +	 printf("callee type %s does not match return storage type %s\n",
> +	        callee->return_type->name, ir->return_deref->type->name);
> +	 abort();
> +      }
> +   }

I think this could be slightly simplified:

if (ir->return_deref) {
   if (ir->return_deref->type != callee->return_type) {
      printf("callee type %s does not match return storage type %s\n",
             callee->return_type->name, ir->return_deref->type->name);
      abort();
   }
} else if (callee->return_type != glsl_type::void_type) {
   printf("ir_call has non-void callee but no return storage\n");
   abort();
}

but either way.

> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index 09ffdff..d56eb97 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -117,6 +117,15 @@ public:
>  	 sig_iter.next();
>        }
>  
> +      if (ir->return_deref != NULL) {
> +	 ir_variable *const var = ir->return_deref->variable_referenced();

= ir->return_deref->var;

Every time I see variable_referenced(), I think about what kind of
rvalue tree it is and the possibility of a NULL return.

Actually, I think there's a later patch opportunity to simplify this
visitor a bunch of the optimization passes by setting in_assignee
during function out/inout parameter visiting, and then dropping a
bunch of ir_call special cases in favor of the
ir_dereference_variable() visit function with an in_assignee check.

You can bump my Reviewed-by up to the current version.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120329/2b2ebd92/attachment.pgp>