[Mesa-dev] [PATCH 2/4] glsl: Emit function signatures at toplevel, even for built-ins.

[Ian Romanick]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 08/01/2011 04:07 PM, Paul Berry wrote:
> The ast-to-hir conversion needs to emit function signatures in two
> circumstances: when a function declaration (or definition) is
> encountered, and when a built-in function is encountered.
> 
> To avoid emitting a function signature in an illegal place (such as
> inside a function), emit_function() checked whether we were inside a
> function definition, and if so, emitted the signature before the
> function definition.
> 
> However, this didn't cover the case of emitting function signatures
> for built-in functions when those built-in functions are called from
> global scope (e.g. a built-in function called from inside the constant
> integer expression that specifies the length of an array).
> 
> This patch changes emit_function() so that it emits function
> signatures at toplevel in all cases.

There's something about this patch that I don't grok.  The
state->current_function test in emit function exists specifically to
handle calls to functions (built-in or otherwise) at global scope.
Without that, code such as the snippet below would not work, and text in
the commit log seems to indicate that it shouldn't work.  However, I
verified that it does.

#version 120
const float x = cos(3.14159 / 2.0);

> ---
>  src/glsl/ast.h                |    3 +--
>  src/glsl/ast_function.cpp     |    2 +-
>  src/glsl/ast_to_hir.cpp       |   31 ++++++++++++++-----------------
>  src/glsl/glsl_parser_extras.h |    6 ++++++
>  4 files changed, 22 insertions(+), 20 deletions(-)
> 
> diff --git a/src/glsl/ast.h b/src/glsl/ast.h
> index 878f48b..d1de227 100644
> --- a/src/glsl/ast.h
> +++ b/src/glsl/ast.h
> @@ -730,7 +730,6 @@ _mesa_ast_field_selection_to_hir(const ast_expression *expr,
>  				 struct _mesa_glsl_parse_state *state);
>  
>  void
> -emit_function(_mesa_glsl_parse_state *state, exec_list *instructions,
> -	      ir_function *f);
> +emit_function(_mesa_glsl_parse_state *state, ir_function *f);
>  
>  #endif /* AST_H */
> diff --git a/src/glsl/ast_function.cpp b/src/glsl/ast_function.cpp
> index 8bcf48d..34a82f8 100644
> --- a/src/glsl/ast_function.cpp
> +++ b/src/glsl/ast_function.cpp
> @@ -125,7 +125,7 @@ match_function_by_name(exec_list *instructions, const char *name,
>  	    if (f == NULL) {
>  	       f = new(ctx) ir_function(name);
>  	       state->symbols->add_global_function(f);
> -	       emit_function(state, instructions, f);
> +	       emit_function(state, f);
>  	    }
>  
>  	    f->add_signature(sig->clone_prototype(f, NULL));
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index c0524bf..c1f160e 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -66,6 +66,8 @@ _mesa_ast_to_hir(exec_list *instructions, struct _mesa_glsl_parse_state *state)
>  
>     state->current_function = NULL;
>  
> +   state->toplevel_ir = instructions;
> +
>     /* Section 4.2 of the GLSL 1.20 specification states:
>      * "The built-in functions are scoped in a scope outside the global scope
>      *  users declare global variables in.  That is, a shader's global scope,
> @@ -85,6 +87,8 @@ _mesa_ast_to_hir(exec_list *instructions, struct _mesa_glsl_parse_state *state)
>        ast->hir(instructions, state);
>  
>     detect_recursion_unlinked(state, instructions);
> +
> +   state->toplevel_ir = NULL;
>  }
>  
>  
> @@ -2926,23 +2930,16 @@ ast_parameter_declarator::parameters_to_hir(exec_list *ast_parameters,
>  
>  
>  void
> -emit_function(_mesa_glsl_parse_state *state, exec_list *instructions,
> -	      ir_function *f)
> +emit_function(_mesa_glsl_parse_state *state, ir_function *f)
>  {
> -   /* Emit the new function header */
> -   if (state->current_function == NULL) {
> -      instructions->push_tail(f);
> -   } else {
> -      /* IR invariants disallow function declarations or definitions nested
> -       * within other function definitions.  Insert the new ir_function
> -       * block in the instruction sequence before the ir_function block
> -       * containing the current ir_function_signature.
> -       */
> -      ir_function *const curr =
> -	 const_cast<ir_function *>(state->current_function->function());
> -
> -      curr->insert_before(f);
> -   }
> +   /* IR invariants disallow function declarations or definitions
> +    * nested within other function definitions.  But there is no
> +    * requirement about the relative order of function declarations
> +    * and definitions with respect to one another.  So simply insert
> +    * the new ir_function block at the end of the toplevel instruction
> +    * list.
> +    */
> +   state->toplevel_ir->push_tail(f);
>  }
>  
>  
> @@ -3069,7 +3066,7 @@ ast_function::hir(exec_list *instructions,
>  	 return NULL;
>        }
>  
> -      emit_function(state, instructions, f);
> +      emit_function(state, f);
>     }
>  
>     /* Verify the return type of main() */
> diff --git a/src/glsl/glsl_parser_extras.h b/src/glsl/glsl_parser_extras.h
> index 2f4d3cb..fc392da 100644
> --- a/src/glsl/glsl_parser_extras.h
> +++ b/src/glsl/glsl_parser_extras.h
> @@ -129,6 +129,12 @@ struct _mesa_glsl_parse_state {
>      */
>     class ir_function_signature *current_function;
>  
> +   /**
> +    * During AST to IR conversion, pointer to the toplevel IR
> +    * instruction list being generated.
> +    */
> +   exec_list *toplevel_ir;
> +
>     /** Have we found a return statement in this function? */
>     bool found_return;
>  
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk43TfgACgkQX1gOwKyEAw+IrwCfYvuuQJtoS8RpI0lICMoTgYsm
RrcAn0h9erWBQTPL4i8AkXR+J+Byfawi
=oPRj
-----END PGP SIGNATURE-----