[Mesa-dev] [PATCH 4/4] glsl: validate sampler array indexing for 'constant-index-expression'

[Francisco Jerez]

Francisco Jerez <currojerez at riseup.net> writes:

> Tapani Pälli <tapani.palli at intel.com> writes:
>
>> Desktop GLSL < 130 and GLSL ES < 300 allow sampler array indexing where
>> index can contain a loop induction variable. This extra check will warn
>> during linking if some of the indexes could not be turned in to constant
>> expressions.
>>
>> v2: warning instead of error for backends that did not enable
>>     UnrollSamplerArrayDynamicIndexing option (have dynamic indexing)
>>
>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>> Cc: "10.5" and "10.6" <mesa-stable at lists.freedesktop.org>
>> ---
>>  src/glsl/linker.cpp | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 77 insertions(+)
>>
>> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
>> index 9978380..27d7c18 100644
>> --- a/src/glsl/linker.cpp
>> +++ b/src/glsl/linker.cpp
>> @@ -346,6 +346,39 @@ private:
>>     bool uses_non_zero_stream;
>>  };
>>  
>> +/* Class that finds array derefs and check if indexes are dynamic. */
>> +class dynamic_sampler_array_indexing_visitor : public ir_hierarchical_visitor
>> +{
>> +public:
>> +   dynamic_sampler_array_indexing_visitor() :
>> +      dynamic_sampler_array_indexing(false)
>> +   {
>> +   }
>> +
>> +   ir_visitor_status visit_enter(ir_dereference_array *ir)
>> +   {
>> +      if (!ir->variable_referenced())
>> +         return visit_continue;
>> +
>> +      if (!ir->variable_referenced()->type->contains_sampler())
>> +         return visit_continue;
>> +
>> +      if (!ir->array_index->constant_expression_value()) {
>> +         dynamic_sampler_array_indexing = true;
>> +         return visit_stop;
>> +      }
>> +      return visit_continue;
>> +   }
>> +
>> +   bool uses_dynamic_sampler_array_indexing()
>> +   {
>> +      return dynamic_sampler_array_indexing;
>> +   }
>> +
>> +private:
>> +   bool dynamic_sampler_array_indexing;
>> +};
>> +
>>  } /* anonymous namespace */
>>  
>>  void
>> @@ -2736,6 +2769,40 @@ build_program_resource_list(struct gl_context *ctx,
>>      */
>>  }
>>  
>> +/**
>> + * This check is done to make sure we allow only constant expression
>> + * indexing and "constant-index-expression" (indexing with an expression
>> + * that includes loop induction variable).
>> + */
>> +static bool
>> +validate_sampler_array_indexing(struct gl_context *ctx,
>> +                                struct gl_shader_program *prog)
>> +{
>> +   dynamic_sampler_array_indexing_visitor v;
>> +   for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
>> +      if (prog->_LinkedShaders[i] == NULL)
>> +	 continue;
>> +
>> +      bool no_dynamic_indexing =
>> +         ctx->Const.ShaderCompilerOptions[i].UnrollSamplerArrayDynamicIndexing;
>> +
>> +      /* Search for array derefs in shader. */
>> +      v.run(prog->_LinkedShaders[i]->ir);
>> +      if (v.uses_dynamic_sampler_array_indexing()) {
>> +         const char *msg = "sampler arrays indexed with non-constant "
>> +                           "expressions is forbidden in GLSL %s %u";
>
> For the sake of clarity, maybe add that it's sampler array indexing with
> *general* non-constant expressions what is forbidden, loop induction
> variables are allowed and they are technically a kind of non-constant
> expression.
>
>> +         /* Backend has indicated that it has no dynamic indexing support. */
>> +         if (no_dynamic_indexing) {
>> +            linker_error(prog, msg, prog->IsES ? "ES" : "", prog->Version);
>> +            return false;
>> +         } else {
>> +            linker_warning(prog, msg, prog->IsES ? "ES" : "", prog->Version);
>
> It seems a bit mean to spam the user with another warning at link time,
> you've already warned in PATCH 01 that this feature will be removed in
> more recent GLSL versions.  If you drop the warning:
>
Er, nevermind, the warning here is indeed subtly different (you are
doing a kind of indexing not considered under the
constant-index-expression wording), disregard my comment about dropping
the warning, it seems fine to warn the user twice.

> Reviewed-by: Francisco Jerez <currojerez at riseup.net>
>
>> +         }
>> +      }
>> +   }
>> +   return true;
>> +}
>> +
>>  
>>  void
>>  link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
>> @@ -2954,6 +3021,16 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
>>        lower_const_arrays_to_uniforms(prog->_LinkedShaders[i]->ir);
>>     }
>>  
>> +   /* Validation for special cases where we allow sampler array indexing
>> +    * with loop induction variable. This check emits a warning or error
>> +    * depending if backend can handle dynamic indexing.
>> +    */
>> +   if ((!prog->IsES && prog->Version < 130) ||
>> +       (prog->IsES && prog->Version < 300)) {
>> +      if (!validate_sampler_array_indexing(ctx, prog))
>> +         goto done;
>> +   }
>> +
>>     /* Check and validate stream emissions in geometry shaders */
>>     validate_geometry_shader_emissions(ctx, prog);
>>  
>> -- 
>> 2.1.0
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150609/cec44553/attachment.sig>