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

[Eduardo Lima Mitev]

On 11/20/2017 01:54 PM, Timothy Arceri wrote:
> 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.
>

I agree there are good reasons to eventually use the NIR linker for both 
SPIR-V and GLSL.
However, this should be a long term goal.

Having a NIR linker with the same level of completeness and quality as 
the current GLSL linker requires a lot of effort, that will likely span 
months. We certainly don't want to block supporting ARB_gl_spirv until 
we have a regression-free linker for both SPIR-V and GLSL programs.

Having this temporary fork in the driver linking code-path allows us to 
introduce the NIR linker incrementally, activating it on only for a 
subset of cases (SPIR-V programs).

We have also experimented with activating Ian's GLSL-to-SPIR-V generator 
under an environment variable. This will take normal GLSL shaders and 
turn them into SPIR-V shaders, then route them through our SPIR-V paths. 
This could be an
intermediary step before going full NIR linker.

Timothy, what about if I drop this patch from this series, and 
re-introduced it in the next series when the code-path forks really need 
the flag? Would that be ok for you?

Eduardo

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