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

[Ian Romanick]

On 10/19/2016 02:17 PM, Brian Paul wrote:
> On 10/19/2016 02:40 PM, Ian Romanick wrote:
>> 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.
> 
> The shader author is marking the gl_Position VS output as invariant and
> calling ftransform():
> 
> invariant gl_Position;
> void main()
> {
>   gl_Position = ftransform();
> }
> 
> ftransform() expands into gl_ModelviewProjectionMatrix * gl_Vertex.
> Then, afaict, the propagation pass marks gl_ModelviewProjectionMatrix as
> invariant, but that disagrees with the original declaration of the
> matrix and linking fails.
> 
> That's my superficial understanding of it.
> 
> Do you want me to hold off on pushing the patch?  I'd really like to get
> this or another fix in place.

I'm running this
https://cgit.freedesktop.org/~idr/mesa/commit/?h=jenkins&id=e406683dcc993e55b38170aff698470b28909e2f
through our CI right now.  If there are no regressions and it fixes your
problem, I think it's a better solution.  If it doesn't meet those
criteria, let's go ahead with Ken's patch.

> -Brian
> 
>>> 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);
>>>      }
>>>
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>