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

[Timothy Arceri]

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.

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