[Mesa-dev] [PATCH] glsl/linker: validate attribute aliasing before optimizations

[Tapani Pälli]

On 10/27/18 1:32 AM, Timothy Arceri wrote:
> On Fri, Oct 12, 2018, at 5:04 AM, Tapani Pälli wrote:
>> Patch does a 'dry run' of assign_attribute_or_color_locations before
>> optimizations to catch cases where we have aliasing of unused attributes
>> which is forbidden by the GLSL ES 3.x specifications.
>>
>> We need to run this pass before unused attributes may be removed and with
>> attribute binding information from program, therefore we re-use existing
>> pass in linker rather than attempt to write another one.
>>
>> This fixes WebGL2 test 'gl-bindAttribLocation-aliasing-inactive' and
>> Piglit test 'gles-3.0-attribute-aliasing'.
>>
>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106833
>> ---
>>   src/compiler/glsl/linker.cpp | 31 ++++++++++++++++++++++++++++---
>>   1 file changed, 28 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
>> index 2f4c8860547..905d4b3cbed 100644
>> --- a/src/compiler/glsl/linker.cpp
>> +++ b/src/compiler/glsl/linker.cpp
>> @@ -2709,7 +2709,8 @@ static bool
>>   assign_attribute_or_color_locations(void *mem_ctx,
>>                                       gl_shader_program *prog,
>>                                       struct gl_constants *constants,
>> -                                    unsigned target_index)
>> +                                    unsigned target_index,
>> +                                    bool do_assignment)
>>   {
>>      /* Maximum number of generic locations.  This corresponds to either the
>>       * maximum number of draw buffers or the maximum number of generic
>> @@ -3072,6 +3073,9 @@ assign_attribute_or_color_locations(void *mem_ctx,
>>         num_attr++;
>>      }
>>   
>> +   if (!do_assignment)
>> +      return true;
>> +
>>      if (target_index == MESA_SHADER_VERTEX) {
>>         unsigned total_attribs_size =
>>            util_bitcount(used_locations &
>> SAFE_MASK_FROM_INDEX(max_index)) +
>> @@ -4780,12 +4784,12 @@ link_varyings_and_uniforms(unsigned first,
>> unsigned last,
>>      }
>>   
>>      if (!assign_attribute_or_color_locations(mem_ctx, prog, &ctx->Const,
>> -                                            MESA_SHADER_VERTEX)) {
>> +                                            MESA_SHADER_VERTEX, true)) {
>>         return false;
>>      }
>>   
>>      if (!assign_attribute_or_color_locations(mem_ctx, prog, &ctx->Const,
>> -                                            MESA_SHADER_FRAGMENT)) {
>> +                                            MESA_SHADER_FRAGMENT, true)) {
>>         return false;
>>      }
>>   
>> @@ -5162,6 +5166,27 @@ link_shaders(struct gl_context *ctx, struct
>> gl_shader_program *prog)
>>            lower_tess_level(prog->_LinkedShaders[i]);
>>         }
>>   
>> +      /* Section 13.46 (Vertex Attribute Aliasing) of the OpenGL ES 3.2
>> +       * specification says:
>> +       *
>> +       *    "In general, the behavior of GLSL ES should not depend on
>> compiler
>> +       *    optimizations which might be implementation-dependent. Name
>> matching
>> +       *    rules in most languages, including C++ from which GLSL ES
>> is derived,
>> +       *    are based on declarations rather than use.
>> +       *
>> +       *    RESOLUTION: The existence of aliasing is determined by
>> declarations
>> +       *    present after preprocessing."
>> +       *
>> +       * Because of this rule, we do a 'dry-run' of attribute
>> assignment for
>> +       * vertex shader inputs here.
>> +       */
>> +      if (i == MESA_SHADER_VERTEX) {
> 
> Can we please add a GLES check to the if above so we can skip this for desktop GL.
> 
> Its a little unfortunate we need this patch but splitting assign_attribute_or_color_locations() in two would get messy so with the above change.
> 
> Reviewed-by: Timothy Arceri <tarceri at itsqueeze.com>

Sure, I'll add a GLES check. Thanks for the review!

> 
>> +         if (!assign_attribute_or_color_locations(mem_ctx, prog, &ctx-
>>> Const,
>> +                                                  MESA_SHADER_VERTEX,
>> false)) {
>> +            goto done;
>> +         }
>> +      }
>> +
>>         /* Call opts before lowering const arrays to uniforms so we can
>> const
>>          * propagate any elements accessed directly.
>>          */
>> -- 
>> 2.17.1
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev