[Mesa-dev] [PATCH] nir/linker: Add nir_link_uniforms()

[Alejandro Piñeiro]

On 30/04/18 13:02, Timothy Arceri wrote:
> On 30/04/18 16:59, Alejandro Piñeiro wrote:
>> On 29/04/18 12:09, Timothy Arceri wrote:
>>> On 29/04/18 19:05, Alejandro Piñeiro wrote:
>>>> On 29/04/18 10:13, Timothy Arceri wrote:
>>>>> On 29/04/18 16:48, Alejandro Piñeiro wrote:
>>>>>> From: Eduardo Lima Mitev <elima at igalia.com>
>>>>>>
>>>>>> This function will be the entry point for linking the uniforms from
>>>>>> the nir_shader objects associated with the gl_linked_shaders of a
>>>>>> program.
>>>>>>
>>>>>> This patch includes initial support for linking uniforms from NIR
>>>>>> shaders. It is tailored for the ARB_gl_spirv needs, and it is far
>>>>>> from
>>>>>> complete, but it should handle most cases of uniforms, array
>>>>>> uniforms, structs, samplers and images.
>>>>>>
>>>>>> There are some FIXMEs related to specific features that will be
>>>>>> implemented in following patches, like atomic counters, UBOs and
>>>>>> SSBOs.
>>>>>>
>>>>>> Also, note that ARB_gl_spirv makes mandatory explicit location for
>>>>>> normal uniforms, so this code only handles uniforms with explicit
>>>>>> location. But there are cases, like uniform atomic counters, that
>>>>>> doesn't have a location from the OpenGL point of view (they have a
>>>>>> binding), but that Mesa assign internally a location. That will be
>>>>>> handled on following patches.
>>>>>>
>>>>>> A nir_linker.h file is also added. More NIR-linking related API will
>>>>>> be added in subsequent patches and those will include stuff from
>>>>>> Mesa,
>>>>>> so reusing nir.h didn't seem a good idea.
>>>>>
>>>>> These files should actually be src/compiler/glsl/nir_link_uniforms.c
>>>>> etc these are not general purpose nir helpers they are GLSL specific.
>>>>
>>>> Yes, it is true that are not general purpose nir helpers, but they are
>>>> not GLSL specific either. As mentioned on the commit message and on
>>>> the
>>>> introductory comment, it is heavily tailored for ARB_gl_spirv, so they
>>>> are SPIR-V specific.
>>>
>>> But why? Why not try to make it reusable as a linker for GLSL? What
>>> exactly is tailored for ARB_gl_spirv? And does this really block us
>>> reusing it for GLSL?
>>>
>>> I've expressed my opinion on this long ago, we are very close to
>>> being able to implement a NIR linker for GLSL, spending a little
>>> effort designing this linker code as share-able seems like a no
>>> brainer to me.
>>
>> Yes, it is true that we didn't explain in detail that decision on the
>> mailing list. We briefly mentioned that on the patches that we were
>> sending to the list, and explained on my presentation on FOSDEM [1],
>> where Nicolai mentioned during the Q&A section that they agreed to us
>> (at least with going for a nir-based linking). In fact, at the
>> beginning we also hoped that this work would be more aligned with a
>> more general nir-linker, and more easily reused for a nir-based GLSL
>> linker, but our opinion changed as we started to code the needs for
>> this extension.
>>
>> In summary the main reason are the names. Right now GLSL linking is
>> based on the names. Uniform name, ubo name, and so on. So for
>> example, from link_uniform_blocks.cpp:
>>
>>     /* This hash table will track all of the uniform blocks that have
>> been
>>      * encountered.  Since blocks with the same block-name must be
>> the same,
>>      * the hash is organized by block-name.
>>      */
>>
>> And most the validation rules on GLSL are based, or include somehow,
>> the name of the variables.
>>
>> But on ARB_gl_spirv, everything should work without names. Read as
>> example issues 12, 21 and 22 of the spec [2] as a reference. In fact
>> names are optional even if the SPIR-V include them (see issue 19). So
>> the linking for nir shaders coming from ARB_gl_spirv should work
>> based on the location, binding, offsets, etc. With the previous
>> example, ubos are linked using the binding. So explicit bindings are
>> now mandatory (so the validation rule here change, as not having a
>> explicit binding can be raised as an error).
>>
>> So in order to work for ARB_gl_spirv it should work without names. In
>> order to reuse it for GLSL it should work with names, in fact based
>> on them. Validation rules for both are different. And getting both
>> working at the same time would just add a complexity that IMHO we
>> don't need right now. We already have a GLSL linker, so it is not
>> really worth right now to get this new NIR linker to work there too.
>> Right now we are focusing on getting ARB_gl_spirv linkage working.
>>
>> Another reason is that ARB_gl_spirv GLSL supported features are not
>> exactly any existing GLSL. It is based on GL_KHR_vulkan_glsl, but at
>> the same time, slightly tweaked. See "Differences Relative to Other
>> Specifications" on the ARB_gl_spirv spec [2].
>>
>> So perhaps in the future some of this work could be reused for
>> nir-base linker for GLSL. But, IMHO, that shouldn't be one of the
>> features driving the development. I think that we should work first
>> on what it is not supported.
>
> None of this looks like anything that should block reuse of at least
> large parts of a NIR linker between the two IRs. 


> Names can be easily mapped to numerical ids which we assign in glsl
> either internally or via the API/shader in one why our another.

This seems like an oversimplification to me. Yes names can be easily
mapped. So for example, on GLSL that numerical id would be used instead
of the name for linking and validate. But still, on GLSL you would need
to use that mapped id, while on SPIR-V you would need to use location or
binding depending on the context.

And again, ARB_gl_spirv implements some specific rules that were not the
same that those on GLSL. Being bold, if instead of having a spirv to nir
pass, we had a spirv to glsl-ir pass, I think that a lot of the new
support would be implemented as new  ir-linking code. Yes re-using a lot
of the utility ir methods, but somewhat independent.

>
> I'm not saying you must implement a GLSL linker right now, I'm just
> saying it would be a good idea to design the linker in a way you think
> would make it easy to code share in future.

And Im not saying that we think that a NIR-based GLSL linker can't be
based on what we are doing right now in the future. And I think that
eventually it would be good, just to check if avoiding having two
different linkers can be avoided. I'm just saying that at the beginning,
when we tried to share as much as possible, and to abstract as much as
possible for both worlds, development progress stalled.  So we decided
that it was better to prioritize, and focus first on the feature that
was not implemented.

>
>
> <snip>
>
> The snipped section was covered in a different reply thread.
>