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

[Tapani Pälli]

Hi Emil;

I got r-b now for all the patches considering this series. At least one 
of these (i965: use EmitNoIndirectSampler for gen < 7) does not apply as 
is to the 10.6 tree as it is due to other changes.

How does the process go, should I sent a separate patch for 10.6 tree 
(there the change should happen in a different file) or do we try to 
pull in other patches to make this work?

Thanks;

// Tapani


On 06/25/2015 05:24 PM, Emil Velikov wrote:
> Hi gents,
>
> On 9 June 2015 at 14:09, Francisco Jerez <currojerez at riseup.net> wrote:
>> 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>
>>>
> If I understand things correctly the series (patches 1/4 and 4/4 from
> stable pov) are reviewed but are not in master. Are there any
> obstacles/objections against doing so ?
>
> Thanks
> Emil
>