[Mesa-dev] [PATCH] glsl: don't eliminate texcoords that can be set by GL_COORD_REPLACE

[Marek Olšák]

Hi Ian,

In case you're interested, I have noticed we have no piglit tests for
GL_ARB_base_instance. For example, baseinstance shouldn't affect
gl_InstanceID, which is currently broken in radeonsi.

Marek

On Sun, Aug 18, 2013 at 5:23 AM, Ian Romanick <idr at freedesktop.org> wrote:
> On 08/09/2013 01:40 PM, Marek Olšák wrote:
>>
>> Tested by examining generated TGSI shaders from piglit/glsl-routing.
>>
>> Cc: mesa-stable at lists.freedesktop.org
>
>
> This patch was in my review-queue, and I forgot about it.  Sorry. :(
>
> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
>
> Since this also fixes an application, do you have any idea what could be
> done to make a piglit test to reproduce the failure?  We have some folks
> writing piglit tests for us this summer, and this sounds like a good one for
> them. :)
>
>
>> ---
>>   src/glsl/ir_optimization.h             |  2 +-
>>   src/glsl/linker.cpp                    |  6 +++---
>>   src/glsl/opt_dead_builtin_varyings.cpp | 27 +++++++++++++++++++--------
>>   3 files changed, 23 insertions(+), 12 deletions(-)
>>
>> diff --git a/src/glsl/ir_optimization.h b/src/glsl/ir_optimization.h
>> index a61227b..b79c2b7 100644
>> --- a/src/glsl/ir_optimization.h
>> +++ b/src/glsl/ir_optimization.h
>> @@ -77,7 +77,7 @@ bool do_copy_propagation(exec_list *instructions);
>>   bool do_copy_propagation_elements(exec_list *instructions);
>>   bool do_constant_propagation(exec_list *instructions);
>>   void do_dead_builtin_varyings(struct gl_context *ctx,
>> -                              exec_list *producer, exec_list *consumer,
>> +                              gl_shader *producer, gl_shader *consumer,
>>                                 unsigned num_tfeedback_decls,
>>                                 class tfeedback_decl *tfeedback_decls);
>>   bool do_dead_code(exec_list *instructions, bool
>> uniform_locations_assigned);
>> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
>> index d36f627..f87ae0e 100644
>> --- a/src/glsl/linker.cpp
>> +++ b/src/glsl/linker.cpp
>> @@ -2091,7 +2091,7 @@ link_shaders(struct gl_context *ctx, struct
>> gl_shader_program *prog)
>>               goto done;
>>         }
>>
>> -      do_dead_builtin_varyings(ctx, sh->ir, NULL,
>> +      do_dead_builtin_varyings(ctx, sh, NULL,
>>                                  num_tfeedback_decls, tfeedback_decls);
>>
>>         demote_shader_inputs_and_outputs(sh, ir_var_shader_out);
>> @@ -2106,7 +2106,7 @@ link_shaders(struct gl_context *ctx, struct
>> gl_shader_program *prog)
>>          */
>>         gl_shader *const sh = prog->_LinkedShaders[first];
>>
>> -      do_dead_builtin_varyings(ctx, NULL, sh->ir,
>> +      do_dead_builtin_varyings(ctx, NULL, sh,
>>                                  num_tfeedback_decls, tfeedback_decls);
>>
>>         demote_shader_inputs_and_outputs(sh, ir_var_shader_in);
>> @@ -2130,7 +2130,7 @@ link_shaders(struct gl_context *ctx, struct
>> gl_shader_program *prog)
>>                   tfeedback_decls, gs_input_vertices))
>>            goto done;
>>
>> -      do_dead_builtin_varyings(ctx, sh_i->ir, sh_next->ir,
>> +      do_dead_builtin_varyings(ctx, sh_i, sh_next,
>>                   next == MESA_SHADER_FRAGMENT ? num_tfeedback_decls : 0,
>>                   tfeedback_decls);
>>
>> diff --git a/src/glsl/opt_dead_builtin_varyings.cpp
>> b/src/glsl/opt_dead_builtin_varyings.cpp
>> index 2e813d2..6745d5c 100644
>> --- a/src/glsl/opt_dead_builtin_varyings.cpp
>> +++ b/src/glsl/opt_dead_builtin_varyings.cpp
>> @@ -409,7 +409,7 @@ lower_texcoord_array(exec_list *ir, const
>> varying_info_visitor *info)
>>
>>   void
>>   do_dead_builtin_varyings(struct gl_context *ctx,
>> -                         exec_list *producer, exec_list *consumer,
>> +                         gl_shader *producer, gl_shader *consumer,
>>                            unsigned num_tfeedback_decls,
>>                            tfeedback_decl *tfeedback_decls)
>>   {
>> @@ -431,44 +431,55 @@ do_dead_builtin_varyings(struct gl_context *ctx,
>>      varying_info_visitor consumer_info(ir_var_shader_in);
>>
>>      if (producer) {
>> -      producer_info.get(producer, num_tfeedback_decls, tfeedback_decls);
>> +      producer_info.get(producer->ir, num_tfeedback_decls,
>> tfeedback_decls);
>>
>>         if (!consumer) {
>>            /* At least eliminate unused gl_TexCoord elements. */
>>            if (producer_info.lower_texcoord_array) {
>> -            lower_texcoord_array(producer, &producer_info);
>> +            lower_texcoord_array(producer->ir, &producer_info);
>>            }
>>            return;
>>         }
>>      }
>>
>>      if (consumer) {
>> -      consumer_info.get(consumer, 0, NULL);
>> +      consumer_info.get(consumer->ir, 0, NULL);
>>
>>         if (!producer) {
>>            /* At least eliminate unused gl_TexCoord elements. */
>>            if (consumer_info.lower_texcoord_array) {
>> -            lower_texcoord_array(consumer, &consumer_info);
>> +            lower_texcoord_array(consumer->ir, &consumer_info);
>>            }
>>            return;
>>         }
>>      }
>>
>> -   /* Eliminate the varyings unused by the other shader. */
>> +   /* Eliminate the outputs unused by the consumer. */
>>      if (producer_info.lower_texcoord_array ||
>>          producer_info.color_usage ||
>>          producer_info.has_fog) {
>> -      replace_varyings_visitor(producer,
>> +      replace_varyings_visitor(producer->ir,
>>                                  &producer_info,
>>                                  consumer_info.texcoord_usage,
>>                                  consumer_info.color_usage,
>>                                  consumer_info.has_fog);
>>      }
>>
>> +   /* The gl_TexCoord fragment shader inputs can be initialized
>> +    * by GL_COORD_REPLACE, so we can't eliminate them.
>> +    *
>> +    * This doesn't prevent elimination of the gl_TexCoord elements which
>> +    * are not read by the fragment shader. We want to eliminate those
>> anyway.
>> +    */
>> +   if (consumer->Type == GL_FRAGMENT_SHADER) {
>> +      producer_info.texcoord_usage = (1 << MAX_TEXTURE_COORD_UNITS) - 1;
>> +   }
>> +
>> +   /* Eliminate the inputs uninitialized by the producer. */
>>      if (consumer_info.lower_texcoord_array ||
>>          consumer_info.color_usage ||
>>          consumer_info.has_fog) {
>> -      replace_varyings_visitor(consumer,
>> +      replace_varyings_visitor(consumer->ir,
>>                                  &consumer_info,
>>                                  producer_info.texcoord_usage,
>>                                  producer_info.color_usage,
>>
>