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

[Timothy Arceri]

On 20/11/17 21:56, Eduardo Lima Mitev wrote:
> 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.

Why do this for spriv only? Why not us the new nir linker for GLSL also? 
There are many go reasons for doing so.

> 
> 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;
>>>>>   };
>>>>>     /**
>>>>>
>>>>
>>>
>>
>