[Mesa-dev] [PATCH 3/7] linker: Fold set_uniform_binding into call site

[Kenneth Graunke]

On 04/10/2014 11:42 AM, Ian Romanick wrote:
> On 04/09/2014 08:02 AM, Kenneth Graunke wrote:
>> On 04/04/2014 02:01 PM, Ian Romanick wrote:
>>> From: Ian Romanick <ian.d.romanick at intel.com>
>>>
>>> In the next patch, we'll see that using
>>> gl_shader_program::UniformStorage is not correct for uniform blocks.
>>> That means we can't use ::UniformStorage to select between the sampler
>>> path and the block path.  Instead we want to just use the type of the
>>> variable.  That's never passed to set_uniform_binding, and it's easier
>>
>> Ehhhmm.....then....
> 
> "That" in this instance the variable, not the variable's type.  I'll
> update the commit message.  More reply below.
> 
>>> to just remove the function (especially for later patches in the series)
>>> than to add another parameter.
>>>
>>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76323
>>> Cc: "10.1" <mesa-stable at lists.freedesktop.org>
>>> Cc: github at socker.lepus.uberspace.de
>>> ---
>>>  src/glsl/link_uniform_initializers.cpp | 33 ++++++++++++---------------------
>>>  1 file changed, 12 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/src/glsl/link_uniform_initializers.cpp b/src/glsl/link_uniform_initializers.cpp
>>> index 6f15e69..bbdeec9 100644
>>> --- a/src/glsl/link_uniform_initializers.cpp
>>> +++ b/src/glsl/link_uniform_initializers.cpp
>>> @@ -151,25 +151,6 @@ set_block_binding(void *mem_ctx, gl_shader_program *prog,
>>>  }
>>>  
>>>  void
>>> -set_uniform_binding(void *mem_ctx, gl_shader_program *prog,
>>> -                    const char *name, const glsl_type *type, int binding)
>>
>> ...what exactly is this?                 ^^^^^^^^^^^^^^^^^^^^^
>>
>>> -{
>>> -   struct gl_uniform_storage *const storage =
>>> -      get_storage(prog->UniformStorage, prog->NumUserUniformStorage, name);
>>> -
>>> -   if (storage == NULL) {
>>> -      assert(storage != NULL);
>>> -      return;
>>> -   }
>>> -
>>> -   if (storage->type->is_sampler()) {
>>> -      set_sampler_binding(mem_ctx, prog, name, type, binding);
>>> -   } else if (storage->block_index != -1) {
>>> -      set_block_binding(mem_ctx, prog, name, type, binding);
>>> -   }
>>> -}
>>> -
>>> -void
>>>  set_uniform_initializer(void *mem_ctx, gl_shader_program *prog,
>>>  			const char *name, const glsl_type *type,
>>>  			ir_constant *val)
>>> @@ -268,8 +249,18 @@ link_set_uniform_initializers(struct gl_shader_program *prog)
>>>  	    mem_ctx = ralloc_context(NULL);
>>>  
>>>           if (var->data.explicit_binding) {
>>> -            linker::set_uniform_binding(mem_ctx, prog, var->name,
>>> -                                        var->type, var->data.binding);
>>> +            const glsl_type *const type = var->type;
>>
>> Here you're using type, which is var->type, which is exactly what we
>> were already passing.
>>
>> AFAICT all you needed to do was change:
>>
>>    if (storage->type->is_sampler())
>>
>> to
>>
>>     if (type->is_sampler() || (type->is_array() &&
>> type->fields.array->is_sampler()))
>>
>> in set_uniform_binding.
> 
> That would resolve this issue, but patch 5 needs the interface type and
> not the variable's type.  The only way to get the interface type is from
> the variable.  Splitting it out here, then passing the interface type
> instead of the variable's type in a later patch resulted in what I
> believed to be better code.
> 
> Sound reasonable?

Definitely.  Sorry I wasn't clear - although I thought these changes
were unnecessary, I wasn't opposed to them.  They seem worth doing
regardless.

I believe you have my R-b for the whole series.

Thanks Ian!

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140410/86550c1e/attachment.sig>