[Mesa-dev] [Mesa-stable] [PATCH 2/2] glsl: validate sampler array indexing for 'constant-index-expression'
Francisco Jerez
currojerez at riseup.net
Tue May 26 16:30:13 PDT 2015
Ian Romanick <idr at freedesktop.org> writes:
> On 05/26/2015 02:04 PM, Francisco Jerez wrote:
>> Ian Romanick <idr at freedesktop.org> writes:
>>
>>> On 05/26/2015 02:53 AM, Tapani Pälli wrote:
>>>> Hello;
>>>>
>>>> I'd like to ping if this approach would be ok. We've had some
>>>> discussions with Curro about it and overall it would seem nicer to move
>>>> this check to happen at compile time. However, this seems quite a
>>>> problematic move. I'll try explain below why;
>>>>
>>>> The overall problem with the failing use cases in the bug is that loop
>>>> unroll does not happen. It does not happen because loop analysis does
>>>> not know how to deal with functions and there is a texture2D call inside
>>>> the loop.
>>>>
>>>> Now what follows is that because unroll does not happen the array index
>>>> (loop induction variable) does not become constant during compilation,
>>>> this will happen only after linking (where unroll finally happens due to
>>>> function inlining which allows further optimization).
>>>>
>>>> I have a hacky patch where I force unroll to happen early when only
>>>> builtin calls are found inside loop and it works *but* unfortunately it
>>>> does not help since in the unrolled result we still have sampler array
>>>> indexing with a non-constant variable 'i', it will be constant only
>>>> later after linking phase when function inlining and further
>>>> optimizations happen. It looks like this (I modified ir print output a
>>>> bit to fit in email):
>>>>
>>>> 1st round:
>>>> assign var_ref i constant_int 0
>>>> call texture2D (constant int 0)
>>>>
>>>> 2nd round:
>>>> assign var_ref i (var_ref i + constant_int 1)
>>>> call texture2D (var_ref i)
>>>>
>>>> So at this point I got a bit tired of this approach. IMO linker check is
>>>> sufficient and according the spec. Spec does not explicitly specify a
>>>> compiler or linker error for this case but it does say:
>>>
>>> I agree. We could handle GLSL ES at compile time because there is only
>>> one compilation unit per stage, but I'm not convinced handling it
>>> special is worth any effort.
>>
>> I agree with both of you that it's not too important whether this
>> validation happens at compile time or link time, what I find worrying is
>> that we currently have no guarantee that sampler indexing expression of
>> the form given by the spec (a "constant-index-expression") will actually
>> be lowered into a constant by link time, so the check introduced in this
>> patch may give a false positive in cases where the array index has the
>> allowed form, like:
>>
>> | sampler2D tex[N];
>> |
>> | for (i = 0; i < M; i++) {
>> | vec4 x = texture(tex[some_complex_constant_expression_of(i)], ...);
>> | // Very many instructions here, so the loop unrolling pass won't
>> | // have the temptation of unrolling the loop even after linking.
>> | }
>>
>> Admittedly without this check the situation was even worse because the
>> indexing expression would most likely not have been in the form expected
>> by the back-end, so it could have crashed or misrendered at a later
>> point, even though this is a required feature of GLSL ES <3.00.
>>
>> IMHO the loop unrolling pass needs to be fixed to consider sampler
>> indexing with a constant-index-expression (or some easier superset of
>> that, like arbitrary non-constant expressions) as a kind of unsupported
>> array indexing like it already does for other cases, otherwise the
>> valid programs may fail to compile or not depending on the outcome of
>> the loop unrolling heuristic.
>
> I think "need" is perhaps too strong. The point that you've hit on here
> is, in fact, the reason for the change between GLSL ES 1.00 and GLSL ES
> 3.00. :)
>
> We haven't yet encountered a valid application that has a shader that
> won't compile. At this point I don't think it's likely that we ever will.
>
> - Hardware increasingly "just works," so the restriction is unnecessary.
>
> - The "hard" restriction in GLSL ES 3.00.
>
> - Developer "tribal knowledge" that this is dangerous.
>
> The use of NIR is gradually moving up in the linker pipeline. Even if
> this were moderately important, I don't think it's worth investing
> effort in the existing loop infrastructure. That said, we should keep
> this firmly in mind as the NIR loop-handling infrastructure matures. At
> that point we can probably also revert this change.
>
I was thinking we could just change the loop unrolling pass to force
unroll loops in which a sampler array expression occurs with
non-constant index (which are otherwise disallowed by the spec) if we
are on GLSL ES 1.00. That could lead to slightly more aggressive
unrolling than necessary in some cases, but it would make sure that all
cases mentioned in the spec are covered, and it should be easy to do.
>>> The linker check even here can be somewhat problematic. Back-ends do
>>> additional optimization, so they may be able to make some of these
>>> accesses be non-dynamic. Marek in particular has complained about this
>>> before. Perhaps add a flag to make the error a warning (see also my
>>> comment below)?
>>>
>>>> GLSL ES 1.0.17 spec (4.1.9 Arrays):
>>>>
>>>> "Reading from or writing to an array with a non-constant index that is
>>>> less than zero or greater than or equal to the array's size results in
>>>> undefined behavior. It is platform dependent how bounded this undefined
>>>> behavior may be. It is possible that it leads to instability of the
>>>> underlying system or corruption of memory. However, a particular
>>>> platform may bound the behavior such that this is not the case."
>>>>
>>>> So according to spec, we should not really be checking anything but here
>>>> I'm offering undefined behavior as extra linker check allowed by the
>>>> last clause.
>>>
>>> We have platforms that can fully do dynamic indexing of sampler arrays.
>>> I think the "undefined behavior" on those platforms should be "it just
>>> works," perhaps with a portability warning.
>>>
>>> One other comment far below.
>>>
>>>> Any opinions appreciated;
>>>>
>>>> // Tapani
>>>>
>>>>
>>>> On 05/19/2015 03:01 PM, Tapani Pälli wrote:
>>>>> Desktop GLSL < 130 and GLSL ES < 300 allow sampler array indexing where
>>>>> index can contain a loop induction variable. This extra check makes sure
>>>>> that all these indexes turn in to constant expressions during
>>>>> compilation/linking.
>>>>>
>>>>> 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 | 71
>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>> 1 file changed, 71 insertions(+)
>>>>>
>>>>> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
>>>>> index ecdc025..729b27f 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,34 @@ 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).
>>>>> + */
>>>>> +bool
>>>>> +validate_sampler_array_indexing(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;
>>>>> +
>>>>> + /* Search for array derefs in shader. */
>>>>> + v.run(prog->_LinkedShaders[i]->ir);
>>>>> +
>>>>> + if (v.uses_dynamic_sampler_array_indexing()) {
>>>>> + linker_error(prog,
>>>>> + "sampler arrays indexed with non-constant "
>>>>> + "expressions is forbidden in GLSL %s %u",
>>>>> + prog->IsES ? "ES" : "", prog->Version);
>>>>> + return false;
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + return true;
>>>>> +}
>>>>> +
>>>>>
>>>>> void
>>>>> link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
>>>>> @@ -2948,6 +3009,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 makes sure that all such cases
>>>>> + * have been turned in to constant expressions.
>>>>> + */
>>>>> + if ((!prog->IsES && prog->Version < 130) ||
>>>
>>> What about the gpu_shader5 case?
>>
>> Doesn't ARB_gpu_shader5 require GLSL 1.5 at least? AFAICT it should be
>> fine.
>
> Yes. :)
>
>>>>> + (prog->IsES && prog->Version < 300)) {
>>>>> + if (!validate_sampler_array_indexing(prog))
>>>>> + goto done;
>>>>> + }
>>>>> +
>>>>> /* Check and validate stream emissions in geometry shaders */
>>>>> validate_geometry_shader_emissions(ctx, prog);
>>>>>
>>> _______________________________________________
>>> mesa-stable mailing list
>>> mesa-stable at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/mesa-stable
-------------- 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/20150527/2658437c/attachment-0001.sig>
More information about the mesa-dev
mailing list