[Mesa-dev] [PATCH] glsl/linker: Check the invariance of built-in special variables

[Vadym Shovkoplias]

Hi Tapani,

Thanks for the review! I'll add the needed space.

On Fri, Sep 7, 2018 at 8:16 AM, Tapani Pälli <tapani.palli at intel.com> wrote:

> LGTM
>
> It would be nice to have this as part of 'cross_validate_globals' or some
> other pass but considering how special/specific rules we are dealing with
> here IMO it's fine to have a separate pass for it.
>
> Reviewed-by: Tapani Pälli <tapani.palli at intel.com>
>
> One tiny nit below ..
>
>
> On 08/29/2018 12:16 PM, Vadym Shovkoplias wrote:
>
>>  From Section 4.6.4 (Invariance and Linkage) of the GLSL ES 1.0
>> specification
>>
>>      "The invariance of varyings that are declared in both the vertex and
>>       fragment shaders must match. For the built-in special variables,
>>       gl_FragCoord can only be declared invariant if and only if
>>       gl_Position is declared invariant. Similarly gl_PointCoord can only
>>       be declared invariant if and only if gl_PointSize is declared
>>       invariant. It is an error to declare gl_FrontFacing as invariant.
>>       The invariance of gl_FrontFacing is the same as the invariance of
>>       gl_Position."
>>
>> Fixes:
>>      * glsl-pcoord-invariant.shader_test
>>      * glsl-fcoord-invariant.shader_test
>>      * glsl-fface-invariant.shader_test
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107734
>> Signed-off-by: Vadym Shovkoplias <vadym.shovkoplias at globallogic.com>
>> ---
>>   src/compiler/glsl/linker.cpp | 66 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 66 insertions(+)
>>
>> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
>> index dbd76b7fcc..ca569bbea9 100644
>> --- a/src/compiler/glsl/linker.cpp
>> +++ b/src/compiler/glsl/linker.cpp
>> @@ -1286,6 +1286,66 @@ interstage_cross_validate_uniform_blocks(struct
>> gl_shader_program *prog,
>>      return true;
>>   }
>>   +/**
>> + * Verifies the invariance of built-in special variables.
>> + */
>> +static bool
>> +validate_invariant_builtins(struct gl_shader_program *prog,
>> +                            const gl_linked_shader *vert,
>> +                            const gl_linked_shader *frag)
>> +{
>> +   const ir_variable *var_vert;
>> +   const ir_variable *var_frag;
>> +
>> +   if (!vert|| !frag)
>> +      return true;
>>
>
> space before ||,
>
> I thought it was funny to have this check here since with GLES 1,2 you do
> need both vert and frag to be present. But it seems we do that check only
> later so I think this is fine.
>
>
>
> +
>> +   /*
>> +    * From OpenGL ES Shading Language 1.0 specification
>> +    * (4.6.4 Invariance and Linkage):
>> +    *     "The invariance of varyings that are declared in both the
>> vertex and
>> +    *     fragment shaders must match. For the built-in special
>> variables,
>> +    *     gl_FragCoord can only be declared invariant if and only if
>> +    *     gl_Position is declared invariant. Similarly gl_PointCoord can
>> only
>> +    *     be declared invariant if and only if gl_PointSize is declared
>> +    *     invariant. It is an error to declare gl_FrontFacing as
>> invariant.
>> +    *     The invariance of gl_FrontFacing is the same as the invariance
>> of
>> +    *     gl_Position."
>> +    */
>> +   var_frag = frag->symbols->get_variable("gl_FragCoord");
>> +   if (var_frag && var_frag->data.invariant) {
>> +      var_vert = vert->symbols->get_variable("gl_Position");
>> +      if (var_vert && !var_vert->data.invariant) {
>> +         linker_error(prog,
>> +               "fragment shader built-in `%s' has invariant qualifier, "
>> +               "but vertex shader built-in `%s' lacks invariant
>> qualifier\n",
>> +               var_frag->name, var_vert->name);
>> +         return false;
>> +      }
>> +   }
>> +
>> +   var_frag = frag->symbols->get_variable("gl_PointCoord");
>> +   if (var_frag && var_frag->data.invariant) {
>> +      var_vert = vert->symbols->get_variable("gl_PointSize");
>> +      if (var_vert && !var_vert->data.invariant) {
>> +         linker_error(prog,
>> +               "fragment shader built-in `%s' has invariant qualifier, "
>> +               "but vertex shader built-in `%s' lacks invariant
>> qualifier\n",
>> +               var_frag->name, var_vert->name);
>> +         return false;
>> +      }
>> +   }
>> +
>> +   var_frag = frag->symbols->get_variable("gl_FrontFacing");
>> +   if (var_frag && var_frag->data.invariant) {
>> +      linker_error(prog,
>> +            "fragment shader built-in `%s' can not be declared as
>> invariant\n",
>> +            var_frag->name);
>> +      return false;
>> +   }
>> +
>> +   return true;
>> +}
>>     /**
>>    * Populates a shaders symbol table with all global declarations
>> @@ -5011,6 +5071,12 @@ link_shaders(struct gl_context *ctx, struct
>> gl_shader_program *prog)
>>            lower_named_interface_blocks(mem_ctx,
>> prog->_LinkedShaders[i]);
>>      }
>>   +   if (prog->IsES && prog->data->Version == 100)
>> +      if (!validate_invariant_builtins(prog,
>> +            prog->_LinkedShaders[MESA_SHADER_VERTEX],
>> +            prog->_LinkedShaders[MESA_SHADER_FRAGMENT]))
>> +         goto done;
>> +
>>      /* Implement the GLSL 1.30+ rule for discard vs infinite loops Do
>>       * it before optimization because we want most of the checks to get
>>       * dropped thanks to constant propagation.
>>
>> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>



-- 

Vadym Shovkoplias | Senior Software Engineer
GlobalLogic
P +380.57.766.7667  M +3.8050.931.7304  S vadym.shovkoplias
www.globallogic.com
<http://www.globallogic.com/>
http://www.globallogic.com/email_disclaimer.txt
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180907/ad8ec60a/attachment.html>