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

[Francisco Jerez]

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:

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/8bf86aa4/attachment.sig>