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

[Kenneth Graunke]

On 03/29/2012 09:58 AM, Eric Anholt wrote:
> 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.

Oops.  Fixed, thanks.

>> 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.

Agreed.  Changed to your version.

>> 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.

Yeah, worth looking into.

> You can bump my Reviewed-by up to the current version.

Awesome, thanks!