[Mesa-dev] [PATCH] glsl: fix names in lower_constant_arrays_to_uniforms

[Tapani Pälli]

On 03/19/2015 11:39 PM, Kenneth Graunke wrote:
> On Thursday, March 19, 2015 11:59:50 AM Tapani Pälli wrote:
>> Patch adds a counter around the lowering pass so that arrays
>> from different stages cannot end up having same name for uniform.
>>
>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89590
>> Cc: 10.5 10.4 <mesa-stable at lists.freedesktop.org>
>> ---
>>   src/glsl/ir_optimization.h                  |  2 +-
>>   src/glsl/linker.cpp                         |  5 +++--
>>   src/glsl/lower_const_arrays_to_uniforms.cpp | 13 +++++++------
>>   3 files changed, 11 insertions(+), 9 deletions(-)
>
> This seems a bit complicated - why not just pass in the shader stage,
> and use that in the variable name?

IMO this is not too complicated, it is very close to what we had 
originally, I'm just making the counter the same for all stages. Another 
solution would have been to have a static class member, much less code 
changes but a bit dirty.

> Although I suppose you could have multiple FSes and run into the same
> problem...
>
> Just using %p with the variable would totally solve this problem, but
> it would make the debug output different on each invocation, which is
> unfortunate.

I think it would be good to have always the same output but I'm ok with 
this solution too.

>>
>> diff --git a/src/glsl/ir_optimization.h b/src/glsl/ir_optimization.h
>> index e6939f3..261d2fb 100644
>> --- a/src/glsl/ir_optimization.h
>> +++ b/src/glsl/ir_optimization.h
>> @@ -117,7 +117,7 @@ bool lower_noise(exec_list *instructions);
>>   bool lower_variable_index_to_cond_assign(exec_list *instructions,
>>       bool lower_input, bool lower_output, bool lower_temp, bool lower_uniform);
>>   bool lower_quadop_vector(exec_list *instructions, bool dont_lower_swz);
>> -bool lower_const_arrays_to_uniforms(exec_list *instructions);
>> +bool lower_const_arrays_to_uniforms(exec_list *instructions, unsigned *amount);
>>   bool lower_clip_distance(gl_shader *shader);
>>   void lower_output_reads(exec_list *instructions);
>>   bool lower_packing_builtins(exec_list *instructions, int op_mask);
>> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
>> index 0c44677..a688baa 100644
>> --- a/src/glsl/linker.cpp
>> +++ b/src/glsl/linker.cpp
>> @@ -2693,7 +2693,7 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
>>       * uniforms, and varyings.  Later optimization could possibly make
>>       * some of that unused.
>>       */
>> -   for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
>> +   for (unsigned i = 0, arrays = 0; i < MESA_SHADER_STAGES; i++) {
>>         if (prog->_LinkedShaders[i] == NULL)
>>   	 continue;
>>
>> @@ -2710,7 +2710,8 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
>>                                       ctx->Const.NativeIntegers))
>>   	 ;
>>
>> -      lower_const_arrays_to_uniforms(prog->_LinkedShaders[i]->ir);
>> +      lower_const_arrays_to_uniforms(prog->_LinkedShaders[i]->ir,
>> +                                     &arrays);
>>      }
>>
>>      /* Check and validate stream emissions in geometry shaders */
>> diff --git a/src/glsl/lower_const_arrays_to_uniforms.cpp b/src/glsl/lower_const_arrays_to_uniforms.cpp
>> index 2243f47..49c1f31 100644
>> --- a/src/glsl/lower_const_arrays_to_uniforms.cpp
>> +++ b/src/glsl/lower_const_arrays_to_uniforms.cpp
>> @@ -45,11 +45,11 @@
>>   namespace {
>>   class lower_const_array_visitor : public ir_rvalue_visitor {
>>   public:
>> -   lower_const_array_visitor(exec_list *insts)
>> +   lower_const_array_visitor(exec_list *insts, unsigned *amount)
>>      {
>>         instructions = insts;
>>         progress = false;
>> -      index = 0;
>> +      array_amount = amount;
>>      }
>>
>>      bool run()
>> @@ -63,7 +63,7 @@ public:
>>   private:
>>      exec_list *instructions;
>>      bool progress;
>> -   unsigned index;
>> +   unsigned *array_amount;
>>   };
>>
>>   void
>> @@ -82,7 +82,8 @@ lower_const_array_visitor::handle_rvalue(ir_rvalue **rvalue)
>>
>>      void *mem_ctx = ralloc_parent(con);
>>
>> -   char *uniform_name = ralloc_asprintf(mem_ctx, "constarray__%d", index++);
>> +   char *uniform_name = ralloc_asprintf(mem_ctx, "constarray__%d",
>> +                                        (*array_amount)++);
>>
>>      ir_variable *uni =
>>         new(mem_ctx) ir_variable(con->type, uniform_name, ir_var_uniform);
>> @@ -104,8 +105,8 @@ lower_const_array_visitor::handle_rvalue(ir_rvalue **rvalue)
>>   } /* anonymous namespace */
>>
>>   bool
>> -lower_const_arrays_to_uniforms(exec_list *instructions)
>> +lower_const_arrays_to_uniforms(exec_list *instructions, unsigned *amount)
>>   {
>> -   lower_const_array_visitor v(instructions);
>> +   lower_const_array_visitor v(instructions, amount);
>>      return v.run();
>>   }
>>