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

[Timothy Arceri]

On 27/07/17 17:52, Timothy Arceri wrote:
> 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?

I've done some timing of the perf issues with this fix and it's not 
insignificant.

On my machine I'm seeing over 20% decreases in compiling the deus ex 
shaders (which take 5min + on some machines).

I'm seeing the double free race on an 8 core Ryzen when compiling the 
deus ex shaders with shader db. Hopefully we can pinpoint the exact 
piece that is not getting cloned rather than cloning the whole lot again.


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