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

[Timothy Arceri]

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

FYI

https://patchwork.freedesktop.org/patch/219624/

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

Not a problem.

> 
>>
>> <snip>
> 
>