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

[Tapani Pälli]

ping

On 10/12/18 3:04 PM, 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) {
> +         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.
>          */
>