[Mesa-dev] [PATCH] glsl: Allow any sort of sampler array indexing with GLSL ES < 3.00

[Tapani Pälli]

On 04/08/2015 01:36 AM, Ian Romanick wrote:
> On 04/07/2015 03:22 AM, Francisco Jerez wrote:
>> Tapani Pälli <tapani.palli at intel.com> writes:
>>
>>> From: Kalyan Kondapally <kalyan.kondapally at intel.com>
>>>
>>> Dynamic indexing of sampler arrays is prohibited by GLSL ES 3.00.
>>> Earlier versions allow 'constant-index-expression' indexing, where
>>> index can contain a loop induction variable.
>>>
>>> Patch allows dynamic indexing for sampler arrays when GLSL ES < 3.00.
>>> This change makes 'sampler-array-index.frag' parser test in Piglit
>>> pass + fishgl.com works when running Chrome on OpenGL ES 2.0 backend.
>>>
>>> v2: small change and some more commit message (Tapani)
>>>
>>> Signed-off-by: Kalyan Kondapally <kalyan.kondapally at intel.com>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84225
>>
>> Looks good, but did you check what happens now if the shader uses actual
>> variable indexing (i.e. which lowering cannot turn into a constant) on
>> an implementation that doesn't support it?  Hopefully no crashes or
>> hangs?
>
> I think we should add a post-link check that no dynamic indexing remains
> after all the optimizations are complete.  The intention if the ES2
> language was to allow cases where the dynamic indexing could be
> optimized away.  This was redacted in ES3 because each optimizer was
> differently capable, so a shader that worked on one driver/GPU might
> fail on another... even from the same vendor.
>
> Adding the post-link check should prevent the problems the Curro
> (rightly) worried about, and it should still allow the WebGL demo to work.

I was not sure if this is worth the effort since this path has been 
active for desktop GLSL < 1.30 for quite a long time, but I can take a 
look at adding such check.

>
>>> ---
>>>   src/glsl/ast_array_index.cpp | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/src/glsl/ast_array_index.cpp b/src/glsl/ast_array_index.cpp
>>> index ecef651..b2609b6 100644
>>> --- a/src/glsl/ast_array_index.cpp
>>> +++ b/src/glsl/ast_array_index.cpp
>>> @@ -226,7 +226,7 @@ _mesa_ast_array_index_to_hir(void *mem_ctx,
>>>          * dynamically uniform expression is undefined.
>>>          */
>>>         if (array->type->element_type()->is_sampler()) {
>>> -	 if (!state->is_version(130, 100)) {
>>> +	 if (!state->is_version(130, 300)) {
>>>   	    if (state->es_shader) {
>>>   	       _mesa_glsl_warning(&loc, state,
>>>   				  "sampler arrays indexed with non-constant "
>
> It looks like this is what e3ded7f should have made this code.
>
> Looking at the rest of the surrounding code, I don't think this is quite
> right... at the very least, it's not easy to follow.  You can blame me
> and Paul for that.  I think this is correct and easier to follow:
>
>     if (!state->is_version(400, 0) && !state->ARB_gpu_shader5_enable) {
>        if (state->is_version(130, 300))
>           _mesa_glsl_error(&loc, state,
>                              "sampler arrays indexed with non-constant "
>                              "expressions are forbidden in GLSL %s "
>                              "and later"
>                              state->es_shader ? "ES 3.00" : "1.30");
>        else if (state->es_shader)
>           _mesa_glsl_warning(&loc, state,
>                              "sampler arrays indexed with non-constant "
>                              "expressions are optional in %s and will "
>                              "be forbidden in GLSL ES 3.00 and later"
>                              state->version_string());
>        else
>           _mesa_glsl_warning(&loc, state,
>                              "sampler arrays indexed with non-constant "
>                              "expressions will be forbidden in GLSL "
>                              "1.30 and later");
>     }

OK, thanks!

>>> --
>>> 2.1.0
>>>
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>