[Mesa-dev] [PATCH v2] glsl: builtin: always return clones of the builtins

[Kenneth Graunke]

On Wednesday, March 8, 2017 1:15:04 PM PST Lionel Landwerlin wrote:
> Builtins are created once and allocated using their own private ralloc
> context. When reparenting IR that includes builtins, we might be steal
> bits of builtins. This is problematic because these builtins might now
> be freed when the shader that includes then last is disposed. This
> might also lead to inconsistent ralloc trees/lists if shaders are
> created on multiple threads.
> 
> Rather than including builtins directly into a shader's IR, we should
> include clones of them in the ralloc context of the shader that
> requires them. This fixes double free issues we've been seeing when
> running shader-db on a big multicore (72 threads) server.
> 
> v2: Also rename _mesa_glsl_find_builtin_function_by_name() to better
>     reflect how this function is used. (Ken)
> 
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> ---
>  src/compiler/glsl/ast_to_hir.cpp        |  2 +-
>  src/compiler/glsl/builtin_functions.cpp | 22 +++++++++++++++++-----
>  src/compiler/glsl/builtin_functions.h   |  4 ++--
>  3 files changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp
> index 59d03c9c96..27dc21fffe 100644
> --- a/src/compiler/glsl/ast_to_hir.cpp
> +++ b/src/compiler/glsl/ast_to_hir.cpp
> @@ -5653,7 +5653,7 @@ ast_function::hir(exec_list *instructions,
>     if (state->es_shader && state->language_version >= 300) {
>        /* Local shader has no exact candidates; check the built-ins. */
>        _mesa_glsl_initialize_builtin_functions();
> -      if (_mesa_glsl_find_builtin_function_by_name(name)) {
> +      if (_mesa_glsl_has_builtin_function(name)) {
>           YYLTYPE loc = this->get_location();
>           _mesa_glsl_error(& loc, state,
>                            "A shader cannot redefine or overload built-in "
> diff --git a/src/compiler/glsl/builtin_functions.cpp b/src/compiler/glsl/builtin_functions.cpp
> index e03a50c843..8278c51992 100644
> --- a/src/compiler/glsl/builtin_functions.cpp
> +++ b/src/compiler/glsl/builtin_functions.cpp
> @@ -62,6 +62,7 @@
>  #include "program/prog_instruction.h"
>  #include <math.h>
>  #include "builtin_functions.h"
> +#include "util/hash_table.h"
> 
>  #define M_PIf   ((float) M_PI)
>  #define M_PI_2f ((float) M_PI_2)
> @@ -6002,21 +6003,32 @@ ir_function_signature *
>  _mesa_glsl_find_builtin_function(_mesa_glsl_parse_state *state,
>                                   const char *name, exec_list *actual_parameters)
>  {
> -   ir_function_signature * s;
> +   ir_function_signature *s;
>     mtx_lock(&builtins_lock);
>     s = builtins.find(state, name, actual_parameters);
>     mtx_unlock(&builtins_lock);
> -   return s;
> +
> +   if (s == NULL)
> +      return NULL;
> +
> +   struct hash_table *ht =
> +      _mesa_hash_table_create(NULL, _mesa_hash_pointer, _mesa_key_pointer_equal);
> +   void *ctx = state;

Let's call this mem_ctx or just pass in state directly.  We originally
used "ctx" when developing the compiler out of tree, but now that we're
in Mesa where "ctx" refers to gl_context, we've tried to switch over.

Really awesome work finding this!  Thanks!

Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

> +   ir_function *f = s->function()->clone(ctx, ht);
> +   _mesa_hash_table_destroy(ht, NULL);
> +
> +   return f->matching_signature(state, actual_parameters, true);
>  }
> 
> -ir_function *
> -_mesa_glsl_find_builtin_function_by_name(const char *name)
> +bool
> +_mesa_glsl_has_builtin_function(const char *name)
>  {
>     ir_function *f;
>     mtx_lock(&builtins_lock);
>     f = builtins.shader->symbols->get_function(name);
>     mtx_unlock(&builtins_lock);
> -   return f;
> +
> +   return f != NULL;
>  }
> 
>  gl_shader *
> diff --git a/src/compiler/glsl/builtin_functions.h b/src/compiler/glsl/builtin_functions.h
> index 7ae211b48a..14a52b9402 100644
> --- a/src/compiler/glsl/builtin_functions.h
> +++ b/src/compiler/glsl/builtin_functions.h
> @@ -31,8 +31,8 @@ extern ir_function_signature *
>  _mesa_glsl_find_builtin_function(_mesa_glsl_parse_state *state,
>                                   const char *name, exec_list *actual_parameters);
> 
> -extern ir_function *
> -_mesa_glsl_find_builtin_function_by_name(const char *name);
> +extern bool
> +_mesa_glsl_has_builtin_function(const char *name);
> 
>  extern gl_shader *
>  _mesa_glsl_get_builtin_function_shader(void);
> --
> 2.11.0
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170308/7be509da/attachment.sig>