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

[Timothy Arceri]

On 30/04/18 21:27, Alejandro Piñeiro wrote:
> 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.

Right but this is that same for GLSL, it has these resource ids 
depending on context also.

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

You might be right I could be oversimplifying, you are certainly more 
familiar with the spec than me but I just can't see the differences 
being such a big deal that it would block sharing. I guess we will have 
to wait and see.

> 
>>
>> 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.
>>
> 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>