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 <[email protected]>
---
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 <[email protected]>
+ 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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
