[Mesa-dev] [PATCH 33/40] glsl: create separate 32bit and 64bit versions of shader cache objects

[Nicolai Hähnle]

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