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

[Brian Paul]

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.

-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
>