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

Submitted by Kenneth Graunke on Oct. 19, 2016, 6:11 p.m.

Details

Message ID 20161019181130.4638-1-kenneth@whitecape.org
State New
Headers show
Series "glsl: Skip invariant/precision linker checks for built-in variables." ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Kenneth Graunke Oct. 19, 2016, 6:11 p.m.
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.

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@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 :)

Patch hide | download patch | download mbox

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

Comments

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@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@lists.freedesktop.org>
Reviewed-by: Brian Paul <brianp@vmware.com>
Tested-by: Brian Paul <brianp@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);
>      }
>
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@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);
>     }
>
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@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
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@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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
On Wednesday, October 19, 2016 1:40:39 PM PDT 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.

I agree.  Propagating invariant/precise to ir_variables used in
expressions is pretty bogus.  We should really track this with an
ir_expression::exact flag similar to NIR's "exact" flag on ALU
expressions.  I don't recall why it was done this way.

I've hit problems with invariant on uniforms before.  I tried not
propagating it to uniforms back in the day and Curro convinced me
it was wrong and would break things.

This is a hack - it fixes some cases, but won't fix them all.
Not propagating to uniforms will fix some cases, but I think it
breaks others.  I don't think we have tests for those cases.

--Ken
On 10/19/2016 02:31 PM, Kenneth Graunke wrote:
> On Wednesday, October 19, 2016 1:40:39 PM PDT 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.
> 
> I agree.  Propagating invariant/precise to ir_variables used in
> expressions is pretty bogus.  We should really track this with an
> ir_expression::exact flag similar to NIR's "exact" flag on ALU
> expressions.  I don't recall why it was done this way.
> 
> I've hit problems with invariant on uniforms before.  I tried not
> propagating it to uniforms back in the day and Curro convinced me
> it was wrong and would break things.
> 
> This is a hack - it fixes some cases, but won't fix them all.
> Not propagating to uniforms will fix some cases, but I think it
> breaks others.  I don't think we have tests for those cases.

Right... I think the problem case would be expression trees that involve
only uniforms wouldn't necessarily get the invariant treatment.  If one
shader had

    uniform mat4 u0;
    uniform mat4 u1;
    invariant out vec4 o;
    in vec4 i;

    void main()
    {
        o = u0 * u1 * i;
    }

and the other shader had

    uniform mat4 u0;
    uniform mat4 u1;
    invariant out vec4 o0;
    out vec4 o1;
    in vec4 i;

    void main()
    {
        o0 = u0 * u1 * i;
        o1 = u0 * i;
    }

The 'u0 * u1' part of the invariant expression might get optimized
differently in each shader.

Let's go with Ken's patch for now.
On Wed, Oct 19, 2016 at 3:44 PM, Ian Romanick <idr@freedesktop.org> wrote:

> On 10/19/2016 02:31 PM, Kenneth Graunke wrote:
> > On Wednesday, October 19, 2016 1:40:39 PM PDT 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.
> >
> > I agree.  Propagating invariant/precise to ir_variables used in
> > expressions is pretty bogus.  We should really track this with an
> > ir_expression::exact flag similar to NIR's "exact" flag on ALU
> > expressions.  I don't recall why it was done this way.
> >
> > I've hit problems with invariant on uniforms before.  I tried not
> > propagating it to uniforms back in the day and Curro convinced me
> > it was wrong and would break things.
> >
> > This is a hack - it fixes some cases, but won't fix them all.
> > Not propagating to uniforms will fix some cases, but I think it
> > breaks others.  I don't think we have tests for those cases.
>
> Right... I think the problem case would be expression trees that involve
> only uniforms wouldn't necessarily get the invariant treatment.  If one
> shader had
>
>     uniform mat4 u0;
>     uniform mat4 u1;
>     invariant out vec4 o;
>     in vec4 i;
>
>     void main()
>     {
>         o = u0 * u1 * i;
>     }
>
> and the other shader had
>
>     uniform mat4 u0;
>     uniform mat4 u1;
>     invariant out vec4 o0;
>     out vec4 o1;
>     in vec4 i;
>
>     void main()
>     {
>         o0 = u0 * u1 * i;
>         o1 = u0 * i;
>     }
>
> The 'u0 * u1' part of the invariant expression might get optimized
> differently in each shader.
>

I don't think so.  As soon as o0 gets flagged as invariant, piles of stuff
gets shut off including otherwise "safe" things such as CSE and tree
grafting.
On 10/19/2016 04:52 PM, Jason Ekstrand wrote:
> On Wed, Oct 19, 2016 at 3:44 PM, Ian Romanick <idr@freedesktop.org
> <mailto:idr@freedesktop.org>> wrote:
> 
>     On 10/19/2016 02:31 PM, Kenneth Graunke wrote:
>     > On Wednesday, October 19, 2016 1:40:39 PM PDT 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
>     <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 agree.  Propagating invariant/precise to ir_variables used in
>     > expressions is pretty bogus.  We should really track this with an
>     > ir_expression::exact flag similar to NIR's "exact" flag on ALU
>     > expressions.  I don't recall why it was done this way.
>     >
>     > I've hit problems with invariant on uniforms before.  I tried not
>     > propagating it to uniforms back in the day and Curro convinced me
>     > it was wrong and would break things.
>     >
>     > This is a hack - it fixes some cases, but won't fix them all.
>     > Not propagating to uniforms will fix some cases, but I think it
>     > breaks others.  I don't think we have tests for those cases.
> 
>     Right... I think the problem case would be expression trees that involve
>     only uniforms wouldn't necessarily get the invariant treatment.  If one
>     shader had
> 
>         uniform mat4 u0;
>         uniform mat4 u1;
>         invariant out vec4 o;
>         in vec4 i;
> 
>         void main()
>         {
>             o = u0 * u1 * i;
>         }
> 
>     and the other shader had
> 
>         uniform mat4 u0;
>         uniform mat4 u1;
>         invariant out vec4 o0;
>         out vec4 o1;
>         in vec4 i;
> 
>         void main()
>         {
>             o0 = u0 * u1 * i;
>             o1 = u0 * i;
>         }
> 
>     The 'u0 * u1' part of the invariant expression might get optimized
>     differently in each shader.
> 
> I don't think so.  As soon as o0 gets flagged as invariant, piles of
> stuff gets shut off including otherwise "safe" things such as CSE and
> tree grafting.

Ugh.  Then I think we'll need to have Curro tell us what he thought the
problem was.  I'm not good at these kinds of guessing games.
Ian Romanick <idr@freedesktop.org> writes:

> On 10/19/2016 04:52 PM, Jason Ekstrand wrote:
>> On Wed, Oct 19, 2016 at 3:44 PM, Ian Romanick <idr@freedesktop.org
>> <mailto:idr@freedesktop.org>> wrote:
>> 
>>     On 10/19/2016 02:31 PM, Kenneth Graunke wrote:
>>     > On Wednesday, October 19, 2016 1:40:39 PM PDT 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
>>     <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 agree.  Propagating invariant/precise to ir_variables used in
>>     > expressions is pretty bogus.  We should really track this with an
>>     > ir_expression::exact flag similar to NIR's "exact" flag on ALU
>>     > expressions.  I don't recall why it was done this way.
>>     >
>>     > I've hit problems with invariant on uniforms before.  I tried not
>>     > propagating it to uniforms back in the day and Curro convinced me
>>     > it was wrong and would break things.
>>     >
>>     > This is a hack - it fixes some cases, but won't fix them all.
>>     > Not propagating to uniforms will fix some cases, but I think it
>>     > breaks others.  I don't think we have tests for those cases.
>> 
>>     Right... I think the problem case would be expression trees that involve
>>     only uniforms wouldn't necessarily get the invariant treatment.  If one
>>     shader had
>> 
>>         uniform mat4 u0;
>>         uniform mat4 u1;
>>         invariant out vec4 o;
>>         in vec4 i;
>> 
>>         void main()
>>         {
>>             o = u0 * u1 * i;
>>         }
>> 
>>     and the other shader had
>> 
>>         uniform mat4 u0;
>>         uniform mat4 u1;
>>         invariant out vec4 o0;
>>         out vec4 o1;
>>         in vec4 i;
>> 
>>         void main()
>>         {
>>             o0 = u0 * u1 * i;
>>             o1 = u0 * i;
>>         }
>> 
>>     The 'u0 * u1' part of the invariant expression might get optimized
>>     differently in each shader.
>> 
>> I don't think so.  As soon as o0 gets flagged as invariant, piles of
>> stuff gets shut off including otherwise "safe" things such as CSE and
>> tree grafting.
>
> Ugh.  Then I think we'll need to have Curro tell us what he thought the
> problem was.  I'm not good at these kinds of guessing games.

I don't recall the details of the conversation Ken is referring to, but
avoiding propagation of invariance into uniform variables doesn't sound
like a satisfactory fix, because you're likely to run into the same
problem when invariance is propagated into a variable of any other kind
in one compilation unit but it's not propagated into the same variable
in another compilation unit.  What's the purpose of this
cross-validation of invariance done in the linker?  It seems bogus to me
since the same variable may get the invariant qualifier propagated into
it in some compilation unit but not others (and AFAIK that's fine
according to the spec because invariance is only guaranteed as long as
the whole computation of the variable happens within the same
compilation unit).
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.

Ping.  If a more elaborate fix is a ways off, I'd like to commit Ken's 
patch ASAP.

-Brian