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

[Alejandro Piñeiro]

On 30/04/18 11:53, Timothy Arceri wrote:
> <snip>
> I'll reply to the above snipped section in a separate email.
>
>>>
>>> Ignoring the above issue I'm fairly certain adding a dependency on
>>> core mesa GL to NIR is a no no so either way these files don't
>>> belong in src/compiler/nir
>>
>> But we already have that dependency, right? I mean gl_program have a
>> reference to nir_shader for some time now.
>
> This is largely historical when NIR was created we had no Vulkan
> drivers and it originally lived in src/glsl/nir I have a feeling we
> can get rid of most of these old dependencies by moving some GL only
> helpers out of the src/compiler/nir dir, I might give this a go.

Ok, thanks.

>
>>
>> And in any case, the implementation of ARB_gl_spirv is based on NIR,
>> as right now we don't have an alternative to process SPIR-V. So for
>> example, the following patch (already merged on master) [3]:
>>    mesa/glspirv: Add a _mesa_spirv_to_nir() function
>>    This is basically a wrapper around spirv_to_nir() that includes
>> arguments setup and post-conversion validation.
>>
>> Is at main/glspirv.c, and calls directly spirv_to_nir.
>>
>> Although it is true that code would not be called unless the
>> extension is enabled, it depends on NIR and it is included on core
>> mesa (main/gl_spirv.c).
>>
>> Well, unless I'm misunderstanding what do you mean for core mesa. I'm
>> including mesa/main as core mesa too.
>
> A dependency on NIR from Mesa is fine, I'm saying NIR (the core
> library) shouldn't have dependencies on core Mesa.

Ok, thanks for the explanation.

>
>>
>> In any case, I'm not against moving those files to a different
>> directory if needed, just trying to understand why doesn't belong there.
>
> In patch 12 & 16 you are creating a "util" so you can call things from
> the helpers you are placing in the nir dir but these are GL only
> concepts. The util is not a real util that is shared by the vulkan and
> gl compiler but rather you are working around a dependency problem you
> have created by locating the uniform linker and other helpers inside
> the nir directory. You are simply bundling code into builds of Vulkan
> drivers that will never be used.

Good point, with this directory structure, Vulkan drivers are getting
code that will not use at all.

>
> If you simply put these files inside src/compiler/glsl/ then you don't
> need this util at all. 

True, but (and sorry for being nitpicky), it sounds strange to me to put
code on a glsl directory that are not really related to glsl.

> It would probably make more sense now that this extension has come
> along if the dir was actually src/compiler/gl/ but its not worth the
> hassle of changing that.

Ok, I see your point. Yes, changing the name would be a hassle, and
adding a new directory (so having both src/compiler/gl and
src/compiler/glsl) confusing. So unless someone complains or have a
better idea, we would go for your suggestion. But if you don't mind I
would wait to do the change until we get review on the code itself, and
not only on the placement.

>
> <snip>