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

[Timothy Arceri]

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.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;

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