[Mesa-dev] [PATCH 6/8] glsl: check total count of multi-slot double vertex attribs

[Ian Romanick]

On 04/29/2015 06:14 PM, Dave Airlie wrote:
> From: Dave Airlie <airlied at redhat.com>
> 
> The spec is vague all over the place about this, but this seems
> to be the intent, we can probably make this optional later if
> someone makes hw that cares and writes a driver.
> 
> Basically we need to double count some of the d types but
> only for totalling not for slot number assignment.
> 
> Signed-off-by: Dave Airlie <airlied at redhat.com>
> ---
>  src/glsl/linker.cpp | 40 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index 21fde94..5803f82 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -1975,6 +1975,7 @@ assign_attribute_or_color_locations(gl_shader_program *prog,
>     } to_assign[16];
>  
>     unsigned num_attr = 0;
> +   unsigned total_attribs_size = 0;
>  
>     foreach_in_list(ir_instruction, node, sh->ir) {
>        ir_variable *const var = node->as_variable();
> @@ -2016,12 +2017,41 @@ assign_attribute_or_color_locations(gl_shader_program *prog,
>  	 }
>        }
>  
> +      const unsigned slots = var->type->count_attribute_slots();
> +
> +      /* From GL4.5 core spec, section 11.1.1 (Vertex Attributes):
> +       *
> +       * "A program with more than the value of MAX_VERTEX_ATTRIBS active
> +       * attribute variables may fail to link, unless device-dependent
> +       * optimizations are able to make the program fit within available
> +       * hardware resources. For the purposes of this test, attribute variables
> +       * of the type dvec3, dvec4, dmat2x3, dmat2x4, dmat3, dmat3x4, dmat4x3,
> +       * and dmat4 may count as consuming twice as many attributes as equivalent
> +       * single-precision types. While these types use the same number of
> +       * generic attributes as their single-precision equivalents,
> +       * implementations are permitted to consume two single-precision vectors
> +       * of internal storage for each three- or four-component double-precision
> +       * vector."
> +       * Until someone has a good reason in Mesa, enforce that now.
> +       */
> +      if (target_index == MESA_SHADER_VERTEX) {
> +	 total_attribs_size += slots;
> +	 if (var->type->without_array() == glsl_type::dvec3_type ||
> +	     var->type->without_array() == glsl_type::dvec4_type ||
> +	     var->type->without_array() == glsl_type::dmat2x3_type ||
> +	     var->type->without_array() == glsl_type::dmat2x4_type ||
> +	     var->type->without_array() == glsl_type::dmat3_type ||
> +	     var->type->without_array() == glsl_type::dmat3x4_type ||
> +	     var->type->without_array() == glsl_type::dmat4x3_type ||
> +	     var->type->without_array() == glsl_type::dmat4_type)

I think doing

   if (var->type->without_array()->base_type == GLSL_TYPE_DOUBLE)

should be sufficient here.

Also... we may have discussed this w.r.t. varyings in the original fp64
series, why not just make count_attribute_slots() return 2*count for
doubles?

> +	    total_attribs_size += slots;
> +      }
> +
>        /* If the variable is not a built-in and has a location statically
>         * assigned in the shader (presumably via a layout qualifier), make sure
>         * that it doesn't collide with other assigned locations.  Otherwise,
>         * add it to the list of variables that need linker-assigned locations.
>         */
> -      const unsigned slots = var->type->count_attribute_slots();
>        if (var->data.location != -1) {
>  	 if (var->data.location >= generic_base && var->data.index < 1) {
>  	    /* From page 61 of the OpenGL 4.0 spec:
> @@ -2141,6 +2171,14 @@ assign_attribute_or_color_locations(gl_shader_program *prog,
>        num_attr++;
>     }
>  
> +   if (target_index == MESA_SHADER_VERTEX) {
> +      if (total_attribs_size > max_index) {
> +	 linker_error(prog,
> +		      "attempt to use %d vertex attribute slots only %d available ",
> +		      total_attribs_size, max_index);
> +	 return false;
> +      }
> +   }

Blank line here.

>     /* If all of the attributes were assigned locations by the application (or
>      * are built-in attributes with fixed locations), return early.  This should
>      * be the common case.
>