[Mesa-dev] [PATCH 17/24] mesa/main: Add a 'spirv' flag to gl_shader_program_data

[Eduardo Lima Mitev]

On 11/20/2017 11:31 AM, Timothy Arceri wrote:
> On 20/11/17 19:00, Eduardo Lima Mitev wrote:
>> On 11/16/2017 12:23 AM, Timothy Arceri wrote:
>>> On 16/11/17 00:22, Eduardo Lima Mitev wrote:
>>>> This will be used by the linker code to dfferentiate between
>>>> programs made out of SPIR-V or GLSL shaders.
>>>
>>> So far everywhere this is used it seems you could just do something 
>>> like:
>>>
>>>  if (shProg->_LinkedShaders[stage]->spirv_data)
>>>    ... spriv stuff ...
>>>  else
>>>    ... glsl stuff ...
>>>
>>>
>> This flag is a per-program variable (as oppose to a per-shader one). 
>> While it would be possible to know the type of program (spirv vs. 
>> glsl) indirectly by looking at its linked shaders, I think it is 
>> clearer and more readable (and more formal) to have a flag in the 
>> program data for this.
>
> The flag is per-program but as I said above everywhere you use it you 
> have access to the shader so it's totally unneeded. Adding bools 
> everywhere just ends up with two ways to get to the same solution 
> making things less clear and less readable because the code always 
> ends up using a bool here and a NULL check there. It's hardly a 
> stretch to see that if you have spirv_data than its a spriv shader.
>

Please check these two patches which are part of the larger, in-progress 
work to support gl_spirv:

https://github.com/Igalia/mesa/commit/968c17dd4258af731bf14e5836e5ededced5d6f5
https://github.com/Igalia/mesa/commit/0bf39a1d815fdcb0ba281507a6f2d6d88a6dc1a1

These two explain why we need a a per-program flag to fork code-paths 
between glsl and spirv. In both cases we are not in a per-shader 
context, so we cannot practically use spirv_data from gl_linked_shaders.

Now, if you have a proposal for a different per-program way to do what 
the above two patches intend, I will be happy to consider it.

It is my fault if I didn't explain in the commit log that the flag would 
be needed in the 2nd series. Or better, I should have moved this patch 
to the 2nd series where it is actually used.

Eduardo

> As the person that spent a bunch of time cleaning up all these structs 
> at then end of last year (I believe I landed around 100 patches to 
> clean up the mess) I'm very much against adding unnecessary fields 
> back in here.
>
>>
>> This also allows for releasing the memory of the spirv_data structure 
>> when we don't need it, and still being able to know what kind of 
>> program we have.
>
> Why do you need this though? The backend shouldn't care where it came 
> from once you have compiled it right? You also can't free it until 
> gl_shader is freed in which case the API can't query it anymore either 
> so as far as I can see it's not needed.
>
>>
>> I would personally keep this flag.
>>
>> Are there more opinions about this?
>>
>> Thanks for reviewing, Timothy.
>>
>> Edu
>>
>>>
>>>> ---
>>>>   src/mesa/main/mtypes.h | 6 ++++++
>>>>   1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
>>>> index d624f2cbd19..db9c2e1deaa 100644
>>>> --- a/src/mesa/main/mtypes.h
>>>> +++ b/src/mesa/main/mtypes.h
>>>> @@ -2902,6 +2902,12 @@ struct gl_shader_program_data
>>>>        /* Mask of stages this program was linked against */
>>>>      unsigned linked_stages;
>>>> +
>>>> +   /* Whether the shaders of this program are loaded from SPIR-V 
>>>> binaries
>>>> +    * (all have the SPIR_V_BINARY_ARB state). This was introduced 
>>>> by the
>>>> +    * ARB_gl_spirv extension.
>>>> +    */
>>>> +   bool spirv;
>>>>   };
>>>>     /**
>>>>
>>>
>>
>