On 10/02/17 23:13, Nicolai Hähnle wrote:
On 10.02.2017 13:03, Timothy Arceri wrote:


On 10/02/17 22:29, Nicolai Hähnle wrote:
On 07.02.2017 04:42, Timothy Arceri wrote:
From: Timothy Arceri <timothy.arc...@collabora.com>

Pointers will have different lengths so we simply create a different
sha1 for each platform.

In theory we should be able to share cached shaders as we cache all
pointer as uint64_t however if a pointer is ever added to one of the
structs we write to file with blob_write_bytes() we run the risk of
introducing a bug that would be difficult to reproduce or report from
a user point of veiw. It's also very unlikely that Mesa developers will
regularly regression test the interaction of cache sharing between
platforms.

I think we should avoid storing pointers in the first place...

Open to ideas to do it better but they are used to be able to restore
prog_data in i965 [1].

Hmm, interesting, I agree that that's a somewhat reasonable use.

I'm not convinced though that the rationale of this commit is good. _Really_ storing a pointer is clearly a bug;

I think I see what you mean about storing an enum tag for INACTIVE_UNIFORM_EXPLICIT_LOCATION etc now. I'll do that which will remove the only place a pointer is stored for glsl ir. Thanks.

the i965 code in [1] works because there's manual translation code during the read, which makes things work.

If someone adds a pointer to a struct that is written directly to the disk cache without being aware of the disk cache interaction, then any alignment changes will mean that cross-platform sharing no longer works, but the change will almost certainly have introduced a much bigger bug _anyway_. We may as well increase the visibility of the bug by having shader cache errors.

This brings little-endian vs. big-endian to mind, actually. For those, separate caches seem a good idea to me in terms of code-complexity/performance trade-offs.

Cheers,
Nicolai


[1]
https://github.com/tarceri/Mesa/commit/bd0391b0c9ee56c14ccf5abbff4fad15d9cd70d0



Nicolai

---
 src/compiler/glsl/shader_cache.cpp | 2 +-
 src/compiler/glsl/shader_cache.h   | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/compiler/glsl/shader_cache.cpp
b/src/compiler/glsl/shader_cache.cpp
index 44ca5a4..ddcaeee 100644
--- a/src/compiler/glsl/shader_cache.cpp
+++ b/src/compiler/glsl/shader_cache.cpp
@@ -1348,7 +1348,7 @@ shader_cache_read_program_metadata(struct
gl_context *ctx,
    /* Include bindings when creating sha1. These bindings change the
resulting
     * binary so they are just as important as the shader source.
     */
-   char *buf = ralloc_strdup(NULL, "vb: ");
+   char *buf = ralloc_strdup(NULL, CACHED_PROGRAM"\n vb: ");
prog->AttributeBindings->iterate(create_binding_str, &buf);
    ralloc_strcat(&buf, "fb: ");
prog->FragDataBindings->iterate(create_binding_str, &buf);
diff --git a/src/compiler/glsl/shader_cache.h
b/src/compiler/glsl/shader_cache.h
index 1596c33..2994b66 100644
--- a/src/compiler/glsl/shader_cache.h
+++ b/src/compiler/glsl/shader_cache.h
@@ -27,6 +27,12 @@

 #include "util/disk_cache.h"

+#if __x86_64__
+#define CACHED_PROGRAM "program64:"
+#else
+#define CACHED_PROGRAM "program32:"
+#endif
+
 static uint64_t inline
 ptr_to_uint64_t(void *ptr)
 {


_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev



_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to