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

[Timothy Arceri]

On 09/03/17 18:01, Kenneth Graunke wrote:
> 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.

I know this landed a while back but I'm looking into the compiler perf 
and these function clones popped up.

The thing I don't understand is that the only place that uses bits of 
the builtin is generate_inline() and all of those bits are cloned already.

Do you recall what bits of builtins we being stolen by reparenting the IR?

>> 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
>>
> 
> 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>