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

[Timothy Arceri]

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.

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.


<snip>

The snipped section was covered in a different reply thread.