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

[Ian Romanick]

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?

>> +
>> +            if (type->is_sampler()
>> +                || (type->is_array() && type->fields.array->is_sampler())) {
>> +               linker::set_sampler_binding(mem_ctx, prog, var->name,
>> +                                           type, var->data.binding);
>> +            } else if (var->is_in_uniform_block()) {
>> +               linker::set_block_binding(mem_ctx, prog, var->name,
>> +                                         type, var->data.binding);
>> +            } else {
>> +               assert(!"Explicit binding not on a sampler or UBO.");
>> +            }
>>           } else if (var->constant_value) {
>>              linker::set_uniform_initializer(mem_ctx, prog, var->name,
>>                                              var->type, var->constant_value);
>>
> 
> 


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