[Mesa-dev] [PATCH 2/2] mesa: add hard limits for the number of varyings and uniforms for the linker

[Ian Romanick]

On 11/22/2011 07:54 AM, Marek Olšák wrote:
> On Tue, Nov 22, 2011 at 4:05 PM, Jose Fonseca<jfonseca at vmware.com>  wrote:
>> Marek,
>>
>> I think we should take one of two approaches here:
>> - aim for a short-term workaround, without gallium interface changes:
>>   - e.g., provide to GLSL compiler infinite/huge limits, while advertising to the app the pipe driver ones
>>   - or detect the wine process and advertise bigger limits in the r300 driver
>> - aim for accurately describing the pipe driver compiling abilities
>>   - e.g., allow the state tracker to create shaders that exceed abilities, and force a trial generation and compilation of the TGSI shaders when the GLSL shader is first linked
>>
>> But the hard limit caps interface change you propose below doesn't quite align to neither approach: it is an interface change which doesn't truly help describing the pipe driver compiler abilities any better (the actual maximum constants/inputs depends on the shader and is not a global constant), so it merely provides a short term relief.

All of this discussion is largely moot.  The failure that you're so 
angry about was caused by a bug in the check, not by the check itself. 
That bug has already been fixed (commit 151867b).

The exact same check was previously performed in st_glsl_to_tgsi (or 
ir_to_mesa), and the exact same set of shaders would have been rejected. 
  The check is now done in the linker instead.

> I would gladly commit this patch:
>
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index 3527088..4e1e648 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -1813,6 +1813,9 @@ assign_varying_locations(struct gl_context *ctx,
>         }
>      }
>
> +   /* Enable this once the GLSL compiler can properly count in just
> +    * active array elements, not whole arrays. */
> +#if 0
>      if (ctx->API == API_OPENGLES2 || prog->Version == 100) {
>         if (varying_vectors>  ctx->Const.MaxVarying) {
>           linker_error(prog, "shader uses too many varying vectors "
> @@ -1829,6 +1832,7 @@ assign_varying_locations(struct gl_context *ctx,
>           return false;
>         }
>      }
> +#endif
>
>      return true;
>   }
> @@ -1959,10 +1963,16 @@ check_resources(struct gl_context *ctx, struct
> gl_shader_program *prog)
>                        shader_names[i]);
>         }
>
> +      /* Enable this once the GLSL compiler can properly count in just
> +       * active array elements, not whole arrays. */
> +#if 0
>         if (sh->num_uniform_components>  max_uniform_components[i]) {
>            linker_error(prog, "Too many %s shader uniform components",
>                        shader_names[i]);
>         }
> +#else
> +      (void) max_uniform_components;
> +#endif
>      }
>
>      return prog->LinkStatus;
>
>
> BUT I had a very informative discussion with one Intel developer last
> night and he said we can't change the linker because he's on the ARB.
> :D
>
> Now seriously. I do really care about users. And I'd like to release a
> product which works best for them. Some people obviously don't want me
> to release such a product.

For every shader these patches would allow to run correctly, I can find 
a shader that it will allow that won't (and can't) run correctly.  I can 
also find another driver that advertises the same limits where shaders 
in the first group already don't run.

For example, these patches would allow the following shader, which 
cannot possibly work correctly:

#version 120
uniform float a[gl_MaxUniformComponents + 10];
uniform int i = gl_MaxUniformComponents + 5;

void main()
{
     gl_Position = vec4(a[i]);
}

This shader cannot work, and we've lost the ability to tell the user 
that it cannot work.  Instead of an nice error message about exceeding 
limits, the user gets incorrect rendering, an assertion failure, or a 
GPU hang.  None of these is correct (for any definition of correct) or 
what the user wants.  They're also really hard for anyone (driver 
developer or app developer) to debug or fix.

I'm fairly sure that such a change would cause at least one or two 
OpenGL ES 2.0 conformance tests to fail.  There are a lot of tests there 
that poke at various corner cases of limits.