[Mesa-dev] [PATCH v2 13/21] nir/linker: Add gl_nir_link_uniforms()

[Timothy Arceri]

On 06/06/18 19:10, Neil Roberts wrote:
> Timothy Arceri <tarceri at itsqueeze.com> writes:
> 
>>> +static struct type_tree_entry *
>>> +build_type_tree_for_type(const struct glsl_type *type)
>>> +{
>>
>> Do we really need this? As far as I can tell we walk the types here to
>> build a tree then in nir_link_uniform() we walk the tree. Why not just
>> walk the types directly in nir_link_uniform()?
> 
> The tree is needed as a temporary place to store the next_index and
> detect whether a node in the type tree is encountered for the first time
> or not. The nodes can be encountered multiple times because if there is
> an array of structs or array of arrays then we process the subtree of
> the array once for each element in the array.
> 
> In nir_link_uniform, it effectively _is_ directly walking the glsl_type
> tree. However it simultaneously walks the type_tree tree as well for
> this extra side-band information.
> 
> The GLSL linker handles this instead by using a hash table to store the
> next index (see set_opaque_indices in link_uniforms.cpp). The key for
> the table is the name of the uniform with all array indices removed. I
> suppose we could try to do something similar for the nir linker but it
> would have to use a different key because we don’t have the names.
> However my gut feeling is that that wouldn’t be any more efficient then
> using this side-band tree and the code to generate the hash table key
> looks quite involved.
> 
> If you can think of a simpler way to handle this then of course I am
> open to suggestions.

I still think you might be able to drop the tree without using a hash 
table. Can't you just add an offset field to the state struct and use 
this? You would increment it as you recurse down the arrays/structs via 
nir_link_uniform() and you could  detect the first leaf-node once you 
reach the inner most elements in a way similar to what link_uniforms() 
does in with record_type in program_resource_visitor::recursion. When 
moving to the next uniform you would just reset the offset back to 0.

> 
> Thanks for looking at the patch.
> 
> Regards,
> - Neil
>