[Mesa-dev] [PATCH] glsl: Skip invariant/precision linker checks for built-in variables.

[Ian Romanick]

On 10/19/2016 11:11 AM, Kenneth Graunke wrote:
> Brian found a bug with my "inline built-ins immediately" code for shaders
> which use ftransform() and declare gl_Position invariant:
> 
> https://lists.freedesktop.org/archives/mesa-dev/2016-October/132452.html
> 
> Before my patch, things worked due to a specific order of operations:
> 
> 1. link_intrastage_varyings imported the ftransform function into the VS
> 2. cross_validate_uniforms() ran and signed off that everything matched
> 3. do_common_optimization did both inlining and invariance propagation,
>    making the VS/FS versions of gl_ModelViewProjectionMatrix have
>    different invariant qualifiers...but after the check in step 2,
>    so we never raised an error.
> 
> After my patch, ftransform() is inlined right away, and at compile time,
> do_common_optimization propagates the invariant qualifier to the
> gl_ModelViewProjectionMatrix.  When the linker eventually happens, it
> detects the mismatch.

Why are we marking a uniform as invariant in the first place?  That
sounds boats.

> I can't see any good reason to raise a linker error based on qualifiers
> we internally applied to built-in variables - it's not the application's
> fault.  It's either not a problem, or it's our fault.o
> 
> We should probably rework invariance, but this should keep us limping
> along for now.  It's definitely a hack.
> 
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/compiler/glsl/linker.cpp | 33 +++++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 12 deletions(-)
> 
> Hi Brian,
> 
> I'm on vacation today through Friday, so I likely won't be able to
> push this until next week.  If people are okay with my hack, feel free
> to push it before I get back :)
> 
> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
> index 8599590..66f9e76 100644
> --- a/src/compiler/glsl/linker.cpp
> +++ b/src/compiler/glsl/linker.cpp
> @@ -1038,12 +1038,28 @@ cross_validate_globals(struct gl_shader_program *prog,
>              }
>           }
>  
> -         if (existing->data.invariant != var->data.invariant) {
> -            linker_error(prog, "declarations for %s `%s' have "
> -                         "mismatching invariant qualifiers\n",
> -                         mode_string(var), var->name);
> -            return;
> +         /* Skip invariant/precise checks for built-in uniforms.
> +          * If they're used in an invariant calculation, the invariance
> +          * propagation pass might mark these.  But that's not an error
> +          * on the programmer's part - it's our problem.  It shouldn't
> +          * actually matter anyway, so ignore it.
> +          */
> +         if (var->get_num_state_slots() == 0) {
> +            if (existing->data.invariant != var->data.invariant) {
> +               linker_error(prog, "declarations for %s `%s' have "
> +                            "mismatching invariant qualifiers\n",
> +                            mode_string(var), var->name);
> +               return;
> +            }
> +
> +            if (prog->IsES && existing->data.precision != var->data.precision) {
> +               linker_error(prog, "declarations for %s `%s` have "
> +                            "mismatching precision qualifiers\n",
> +                            mode_string(var), var->name);
> +               return;
> +            }
>           }
> +
>           if (existing->data.centroid != var->data.centroid) {
>              linker_error(prog, "declarations for %s `%s' have "
>                           "mismatching centroid qualifiers\n",
> @@ -1062,13 +1078,6 @@ cross_validate_globals(struct gl_shader_program *prog,
>                           mode_string(var), var->name);
>              return;
>           }
> -
> -         if (prog->IsES && existing->data.precision != var->data.precision) {
> -            linker_error(prog, "declarations for %s `%s` have "
> -                         "mismatching precision qualifiers\n",
> -                         mode_string(var), var->name);
> -            return;
> -         }
>        } else
>           variables->add_variable(var);
>     }
>