[Mesa-dev] [PATCH 3/3] glsl: Perform implicit type conversions on function call out parameters.
Chad Versace
chad at chad-versace.us
Thu Aug 4 19:21:42 PDT 2011
On 08/02/2011 05:38 PM, Paul Berry wrote:
> When an out parameter undergoes an implicit type conversion, we need
> to store it in a temporary, and then after the call completes, convert
> the resulting value. In other words, we convert code like the
> following:
>
> void f(out int x);
> float value;
> f(value);
>
> Into IR that's equivalent to this:
>
> void f(out int x);
> float value;
> int x_converted;
> f(x_converted);
> value = float(x_converted);
>
> This transformation needs to happen during ast-to-IR convertion (as
> opposed to, say, a lowering pass), because it is invalid IR for formal
> and actual parameters to have types that don't match.
It is odd that we generate the post_call_conversions IR during ast-to-hir. It is
even more odd that we generate it in a function named match_function_by_name();
after all, it now does much more than a match.
After exploring the code, though, I discovered that match_function_by_name() is
really a misnomer. It is already doing conversion for 'in' parameters, so it
also seems the proper place to do conversion of 'out' parameters.
>
> Fixes piglit tests spec/glsl-1.20/compiler/qualifiers/out-03.vert and
> spec/glsl-1.20/execution/qualifiers/vs-out-{01,02,03}.shader_test, and
> bug 39651.
The Piglit names will need updating after you rename them. Please rename and
commit the Piglit tests before committing this patch.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=39651
> ---
> src/glsl/ast_function.cpp | 64 +++++++++++++++++++++++++++++++++++++++++---
> 1 files changed, 59 insertions(+), 5 deletions(-)
>
> diff --git a/src/glsl/ast_function.cpp b/src/glsl/ast_function.cpp
> index 8bcf48d..f945a94 100644
> --- a/src/glsl/ast_function.cpp
> +++ b/src/glsl/ast_function.cpp
> @@ -134,6 +134,8 @@ match_function_by_name(exec_list *instructions, const char *name,
> }
> }
>
> + exec_list post_call_conversions;
> +
> if (sig != NULL) {
> /* Verify that 'out' and 'inout' actual parameters are lvalues. This
> * isn't done in ir_function::matching_signature because that function
> @@ -141,6 +143,13 @@ match_function_by_name(exec_list *instructions, const char *name,
> *
> * Also, validate that 'const_in' formal parameters (an extension of our
> * IR) correspond to ir_constant actual parameters.
> + *
> + * Also, perform implicit conversion of arguments. Note: to
> + * implicitly convert out parameters, we need to place them in a
> + * temporary variable, and do the conversion after the call
> + * takes place. Since we haven't emitted the call yet, we'll
> + * place the post-call conversions in a temporary exec_list, and
> + * emit them later.
> */
> exec_list_iterator actual_iter = actual_parameters->iterator();
> exec_list_iterator formal_iter = sig->parameters.iterator();
> @@ -185,8 +194,50 @@ match_function_by_name(exec_list *instructions, const char *name,
> }
>
> if (formal->type->is_numeric() || formal->type->is_boolean()) {
> - ir_rvalue *converted = convert_component(actual, formal->type);
> - actual->replace_with(converted);
> + switch (formal->mode) {
> + case ir_var_in:
> + {
> + ir_rvalue *converted
> + = convert_component(actual, formal->type);
> + actual->replace_with(converted);
> + }
> + break;
> + case ir_var_out:
> + if (actual->type != formal->type)
> + {
> + /* Create a temporary variable to hold the out
> + * parameter. Don't pass a hashtable to clone()
> + * because we don't want this temporary variable to
> + * be in scope.
This comment would be more instructive if it contained this snippet from the
commit message:
As as example, this code
void f(out int x);
float value;
f(value);
is translated into IR equivalent to
void f(out int x);
float value;
int x_converted;
f(x_converted);
value = float(x_converted);
This example goes a long way to helping someone understand how you are juggling
the IR.
> + */
> + ir_variable *tmp = formal->clone(ctx, NULL);
> + tmp->mode = ir_var_auto;
> + instructions->push_tail(tmp);
> + ir_dereference_variable *deref_tmp_1
> + = new(ctx) ir_dereference_variable(tmp);
> + ir_dereference_variable *deref_tmp_2
> + = new(ctx) ir_dereference_variable(tmp);
> + ir_rvalue *converted_tmp
> + = convert_component(deref_tmp_1, actual->type);
> + ir_assignment *assignment
> + = new(ctx) ir_assignment(actual, converted_tmp);
> + post_call_conversions.push_tail(assignment);
> + actual->replace_with(deref_tmp_2);
The construction of post_call_conversions looks right to me.
> + }
> + break;
> + case ir_var_inout:
> + /* Inout parameters should never require conversion,
> + * since that would require an implicit conversion to
> + * exist both to and from the formal parameter type,
> + * and there are no bidirectional implicit
> + * conversions.
> + */
> + assert (actual->type == formal->type);
> + break;
> + default:
> + assert (!"Illegal formal parameter mode");
> + break;
> + }
> }
>
> actual_iter.next();
> @@ -196,11 +247,13 @@ match_function_by_name(exec_list *instructions, const char *name,
> /* Always insert the call in the instruction stream, and return a deref
> * of its return val if it returns a value, since we don't know if
> * the rvalue is going to be assigned to anything or not.
> + *
> + * Also insert any out parameter conversions after the call.
> */
> ir_call *call = new(ctx) ir_call(sig, actual_parameters);
> + ir_dereference_variable *deref;
> if (!sig->return_type->is_void()) {
> ir_variable *var;
> - ir_dereference_variable *deref;
>
> var = new(ctx) ir_variable(sig->return_type,
> ralloc_asprintf(ctx, "%s_retval",
> @@ -215,11 +268,12 @@ match_function_by_name(exec_list *instructions, const char *name,
> var->constant_value = call->constant_expression_value();
>
> deref = new(ctx) ir_dereference_variable(var);
> - return deref;
> } else {
> instructions->push_tail(call);
> - return NULL;
> + deref = NULL;
> }
> + instructions->append_list(&post_call_conversions);
> + return deref;
> } else {
> char *str = prototype_string(NULL, name, actual_parameters);
>
With Ian's formatting fixes, the commit message fix, and my comment addition,
I'm content with this patch, and you can call it
Reviewed-by: Chad Versace <chad at chad-versace.us>
--
Chad Versace
chad at chad-versace.us
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 900 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110804/b4f22a11/attachment-0001.pgp>
More information about the mesa-dev
mailing list