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

[Brian Paul]

On 10/19/2016 12:11 PM, 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://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_archives_mesa-2Ddev_2016-2DOctober_132452.html&d=CwIBAg&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=T0t4QG7chq2ZwJo6wilkFznRSFy-8uDKartPGbomVj8&m=FUsV9E5siNJA_21T5neVxOfd-A-t334aLcd7uj41cy0&s=WGbph-T-_o7nJ30qH0kn674oIEsEM_CRXPSGA0yHNrg&e=
>
> 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.
>
> 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 :)

OK.  Tested and works for me.  I'd also like to tag this for the 13.0 
branch.  If there's no other discussion, I'll push this later.

Cc: "13.0" <mesa-stable at lists.freedesktop.org>
Reviewed-by: Brian Paul <brianp at vmware.com>
Tested-by: Brian Paul <brianp at vmware.com>


Thanks, Ken!


>
> 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);
>      }
>