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

[Timothy Arceri]

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>


> +         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