[Mesa-dev] [PATCH 5/5] Change ir_function_signature::constant_expression_value to run through the function.

[Eric Anholt]

On Fri, 27 Apr 2012 10:28:04 +0200, Olivier Galibert <galibert at pobox.com> wrote:
> That removes code duplication with
> ir_expression::constant_expression_value and builtins/ir/*.

I'm concerned that this will turn things that shouldn't be constant
expressions in GLSL (some user-defined function just using its
(constant) arguments and constants) and treating them as constant
expressions.  Do we have tests for that?

>  src/glsl/ir_constant_expression.cpp |  450 +++++++----------------------------
>  1 file changed, 80 insertions(+), 370 deletions(-)
> 
> diff --git a/src/glsl/ir_constant_expression.cpp b/src/glsl/ir_constant_expression.cpp
> index ee1cc1a..a7aafaa 100644
> --- a/src/glsl/ir_constant_expression.cpp
> +++ b/src/glsl/ir_constant_expression.cpp
> @@ -1050,396 +1050,106 @@ ir_function_signature::constant_expression_value(exec_list *actual_parameters)
>     if (!this->is_builtin)
>        return NULL;
>  
> -   unsigned num_parameters = 0;
> +   /*
> +    * Of the builtin functions, only the texture lookups and the noise
> +    * ones must not be used in constant expressions.  They all include
> +    * specific opcodes so they don't need to be special-cased at this
> +    * point.
> +    */
> +
> +   /* Initialize the table of dereferencable names with the function
> +    * parameters.  Verify their const-ness on the way.
> +    *
> +    * We expect the correctness of the number of parameters to have
> +    * been checked earlier.
> +    */
> +   hash_table *deref_hash = hash_table_ctor(8, hash_table_string_hash,
> +					    hash_table_string_compare);

Our variables don't generally have unique names.  It looks like from
your usage, that you could use pointer_hash/compare and get cheaper
searches plus unique references to variables.

> +   const exec_node *parameter_info = parameters.head;
>  
> -   /* Check if all parameters are constant */
> -   ir_constant *op[3];
>     foreach_list(n, actual_parameters) {
> -      ir_constant *constant = ((ir_rvalue *) n)->constant_expression_value();
> +      ir_constant *constant = ((ir_rvalue *) n)->constant_expression_value(NULL);
>        if (constant == NULL)
>  	 return NULL;
>  
> -      op[num_parameters] = constant;
> +      ir_variable *var = (ir_variable *)parameter_info;
> +      hash_table_insert(deref_hash, constant, var->name);
>  
> -      assert(num_parameters < 3);
> -      num_parameters++;
> +      parameter_info = parameter_info->next;
>     }

Why drop the "check if all parameters are constant" code?  

From the GLSL 1.40 spec:

    "a built-in function call whose arguments are all constant
     expressions, with the exception of the texture lookup functions and
     the noise functions."

> +      case ir_type_assignment: {
> +	 ir_assignment *asg = inst->as_assignment();
> +	 if(asg->condition) {

"if ("

> +
> +	 /* (return (expression))
> +	  *
> +	  * End of the line, compute the result and exit.
> +	  */
> +      case ir_type_return:
> +	 result = inst->as_return()->value->constant_expression_value(deref_hash);
> +	 goto done;
> +
> +	 /* Every other expression type, we drop out.  Maybe some
> +	    builtins will need jumps someday, but not yet. */
> +      default:
> +	 goto done;

Does this patchset actually pass all of piglit?  I don't see ir_if
handling for the builtin function processing here, but I see ifs used in
atan(), for example.
-------------- 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/20120430/fc66afab/attachment.pgp>