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

[Timothy Arceri]

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

So far you have uniform and resource var linking functions. I guess 
UBO's could complicate things but it really feels like these should be 
converted as a preparation series rather than having to come back and 
add additional support later on. It may take a little longer but it 
seems worth it IMO.

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

I'm sorry but I really don't see the advantage of doing this. In the end 
I guess it's up to you how you do it.

> 
> 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?

Seems like a good idea for now.

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