On Fri, 27 Apr 2012 10:28:04 +0200, Olivier Galibert <galib...@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.
pgpYaPDXPz3Ct.pgp
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
